Add SaveMessage component to Header #2133

radavis · 2020-03-25T20:18:35Z

Resolves #2104

This pull request changes...

the 'Saved' message in the Header to be a dynamic string, dependent upon the amount of time that has passed since saving.

This pull request is ready to merge when...

Tests have been updated (and all tests are passing)
This code has been reviewed by someone other than the original author
The experience passes a basic manual accessibility audit (keyboard nav, screenreader, text scaling) OR an exemption is documented
The change has been documented
- ~~Associated OpenAPI documentation has been updated~~
Changelog is updated as appropriate

This feature is done when...

Design has approved the experience
Product has approved the experience

What's missing/incongruent with Issue #2104...

Displaying 'am' & 'pm' instead of 'a.m.' & 'p.m.' Is this acceptable?
The three-letter timezone abbreviation is missing. I can add it, but there are caveats.
~~Testing auto updating isn't done, yet.~~ Done!
~~Fix Header Component snapshot test.~~ Fixed!
~~CircleCI tasks are stuck. Does it need a kick?~~ Opening a PR on this repo, and not a fork, made it work.

… for better errors

radavis · 2020-03-26T17:44:41Z

web/src/components/SaveMessage.test.js

+        );
+        expect(subject.text()).toMatch(/^Last saved/);
+        expect(subject.text()).toMatch(/11:59 am/);
+        expect(subject.text()).toMatch(/\(1 minute ago\)$/);


I might want to condense these expectations to one line. I'm curious which you think is easier to read and easier to update.

expect(subject.text()).toMatch("Last saved 11:59 am (1 minute ago)");

After that, it might be possible to format these remaining tests like on line 11

Example: https://github.com/18F/cms-hitech-apd/compare/d91c94..ce556c

I think I like them separate

cms-eapd-bot · 2020-03-26T17:47:11Z

This deploy was cleaned up.

pphillips-fearless

It says 03:23 (probably shouldn't have the leading zero before the 3)
it doesn't have EDT after pm
the task description says "p.m." but I see "pm" (though I agree that p.m. looks funny)

mgwalker · 2020-03-26T19:37:57Z

Regarding "am" vs "a.m.", I'd recommend having design or product weigh in. Historically we've been pretty flexible about tweaking designs for implementation simplicity. Long-term maintainability beats short-term pixel-perfect designs. 😄

Pinging @beparticular @jjames521 @lauraponce @jeromeleecms if y'all have feedback.

jeromeleecms · 2020-03-26T19:47:14Z

Citing our much-missed alum's (Nicole Fenton) book, Nicely Said, am / pm is a commonly accepted abbreviation used for the web (example provided without periods)... I am good to hold to that guidance...

radavis · 2020-03-26T20:06:11Z

How do you all feel about the 3-character Time Zone being a "must-have?" The displayed time would always be in the client's browser-detected time zone, from my understanding of how the momentjs library works. (I could be wrong about this... Double checking...)

radavis · 2020-03-26T20:14:03Z

Yes.

moment(...) is local mode. Ambiguous input (without offset) is assumed to be local time. Unambiguous input (with offset) is adjusted to local time. -- https://momentjs.com/docs/#/parsing/

Input into the moment library will be adjusted to local time. Calls to output/format time will be in the browser-detected local time.

radavis · 2020-03-26T20:41:12Z

It feels like getting the three-character timezone from the browser is unsupported by the moment library. moment/moment#162

We could investigate this, if we really want to spend the effort. However, it feels like time could be better spent elsewhere...

jeromeleecms · 2020-03-26T20:47:48Z

@radavis Great question - I had originally thought just having a 2 letter time zone appended to the content would be helpful since we don't all work in the same time zone... however... am I understanding that the time zone is automatically localized to client's time zone?

If that's the case, I would be OK with dropping it off entirely -- unless someone has a compelling reason for us to keep it there.

radavis · 2020-03-26T20:53:03Z

I understanding that the time zone is automatically localized to client's time zone?

Yes. As long as the moment docs are telling the truth :)

jeromeleecms · 2020-03-26T20:54:53Z

Alright I'm good with dropping it then!

mgwalker

Code looks good to me. My only feedback is around style choices and consistency, so that's something for you and @pphillips-fearless to decide on. I'm happy to weigh in on why we chose certain approaches (though sometimes it's just... because), but ultimately y'all will be the ones living in this code, so it makes sense for it to bend to your preferences.

mgwalker · 2020-03-26T19:51:54Z

web/src/components/SaveMessage.js

+  },
+});
+
+class SaveMessage extends React.Component {


We've mostly switched over to functional components using React hooks (in this case, useEffect and useState). Since you and @pphillips-fearless are taking over the code, I'll defer to your opinions on which you would prefer, but wanted to point this out for consistency's sake.

Ah, yes. I can definitely change this to a functional component since that will maintain consistency with other components.

I'm just reading up on hooks, not having used them before, but so far I like what I'm reading

mgwalker · 2020-03-26T20:55:14Z

web/src/components/SaveMessage.js

+    // eslint's "yoda": "Expected literal to be on the right side of <="
+    // Which is easier to visualize on a number line, Mr. Yoda?
+    // lowerBound <= object.value() && object.value() < upperBound  // or...
+    if (duration.asMinutes() >= 1 && duration.asDays() < 1) {
+      result = `${result} ${lastSavedMoment.format("h:mm a")}`;
+    }


To be honest, I think it reads better this way. Having the subject of the check come first kind of reads clearer syntactically to me:

if the duration is not more than a minute
vs.
if a minute is more than the duration

lowerBound <= object.value() && object.value() < upperBound could read: is the value between the lower and upper bound

mgwalker · 2020-03-26T20:56:12Z

web/src/components/SaveMessage.js

+    if (duration.asDays() >= 1 && duration.asYears() < 1) {
+      result = `${result} ${lastSavedMoment.format("MMMM D")}`;
+    }
+    if (duration.asYears() >= 1) {
+      result = `${result} ${lastSavedMoment.format("MMMM D, YYYY")}`;
+    }


I think these would be better as else ifs, so the conditionals don't get evaluated if a previous block already executed.

mgwalker · 2020-03-26T20:56:47Z

web/src/components/SaveMessage.js

+  lastSaved: PropTypes.oneOfType([
+    PropTypes.instanceOf(Date),
+    PropTypes.instanceOf(moment),
+    PropTypes.string
+  ].isRequred),


Okay, your propTypes hygiene is way better than mine. Love this.

Cool, cool. I was just thinking of the possible types you could throw at this component. I'm not necessarily testing all of those types, but moment should be able to handle them...

mgwalker · 2020-03-26T21:00:02Z

web/src/components/SaveMessage.js

+SaveMessage.defaultProps = {
+  lastSaved: moment(),
+};


Since lastSaved is required, I don't think a defaultProp is necessary. Elsewhere, we only define defaultProps if the prop is not required. Again deferring to you and @pphillips-fearless's preferences on how you like to write the code, though.

(This is the sort of comment I can imagine leaving lots of over the next few weeks, just pointing out how we've structured things in the past so y'all can make your best decisions about how you want to do things.)

I think the linter didn't like that it didn't have a default prop. I can take it out and verify.

mgwalker · 2020-03-26T21:02:35Z

web/src/components/SaveMessage.test.js

+  let lastSaved;
+  let subject;


Suggested change

let lastSaved;

let subject;

I'd move these closer to the point-of-use, at which point they can be consts. For me, at least, it's clearer that the value is new and I'm not worrying about overwriting some previously existing variable.

mgwalker · 2020-03-26T21:04:14Z

web/src/components/SaveMessage.test.js

+        lastSaved = moment().subtract(seconds, "seconds");
+        subject = shallow(


Suggested change

lastSaved = moment().subtract(seconds, "seconds");

subject = shallow(

const lastSaved = moment().subtract(seconds, "seconds");

const subject = shallow(

This is a stylistic choice, so again something for you and @pphillips-fearless to decide how you want to do it. I'm just imagining some future version of you or another dev looking at this and wondering "where did lastSaved come from?" and then having to search for it. 🤷‍♂

I like using const whenever possible

As the file grows, it could be difficult to find these variable declarations. Your argument makes sense. 👍

mgwalker · 2020-03-26T21:15:48Z

web/src/components/SaveMessage.test.js

+      mockDateNow = jest
+        .spyOn(Date, "now")
+        .mockReturnValueOnce(now)
+        .mockReturnValue(oneMinuteFromNow);


This makes me a little bit nervous because it relies on the internal behavior of moment. Ideally jest.useFakeTimers() would let us initialize it with a timestamp and it would also fake out Date for us (sinon's fake timers work that way), and then we could just fast-forward the timer by 60 seconds. Alas, Jest isn't there...

(Which is all to say, I don't love this but I also think it's probably the best solution.)

This is necessary because the component under test is using moment (which is using Date) under the hood. I can attempt to refactor this and mock moment's functionality, it just might take me a bit longer to figure out.... I agree, it doesn't look pretty.

No no, please don't! It's good as-is. It's a shortcoming of the jest time mocks, not your code!

mgwalker · 2020-03-26T21:28:50Z

web/src/components/SaveMessage.test.js

+      const testName = `when saved ${value} ${timeUnit} ago, it displays "${result}"`
+      test(testName, () => {


Suggested change

const testName = `when saved ${value} ${timeUnit} ago, it displays "${result}"`

test(testName, () => {

test(`when saved ${value} ${timeUnit} ago, it displays "${result}"`, () => {

Another style choice. My preference is not to create variables that are only used once on the immediate next line unless creating them is a long or complex sequence. In the case of strings, I think making them is easy enough to read (especially with template strings!) not to ever bother. My thinking here is that when you create a variable, you're implying something about the lifetime of that value, so my brain stashes it away so I know what it is when I see it later. But then I never see it later. 😛

Can do. I always wrestle with the idea of "Am I doing too much on a single line?," and tend to break things up.

mgwalker

Oops. Meant to mark that as approved.

@radavis Can you add @jeromeleecms and @lauraponce as reviewers on this PR? It'll also need product and design's approval before shipping.

radavis · 2020-03-26T22:05:33Z

Just noticing that I can't get to the deployed site via the cms-eapd-bot's link. I'll investigate this tomorrow in the AM.

jjames521 · 2020-03-26T22:27:32Z

@jeromeleecms think you had referenced the design system being somewhat built off the ideology of Material Design in some ways. In that case am / pm would fit with the system pretty nicely, and as Nicole had said in Nicely Said. it's commonly accepted anyways.

mgwalker

👍

mgwalker · 2020-03-27T17:42:06Z

web/src/components/SaveMessage.test.js

+        const lastSaved = moment().subtract(seconds, "seconds");
+        // https://reactjs.org/docs/test-renderer.html#testrendereract
+        act(() => {
+          subject = create(<SaveMessage lastSaved={lastSaved} />);
+        })
+        expect(subject.toJSON()).toEqual("Saved");


I'm going to open a tech debt issue to come back and think about these tests at some point. These particular ones are a decent candidate for snapshots, for example.

Certainly not something worth changing now and maybe not worth changing at all! But let's put a pin in it and figure it out later. 😄

mgwalker · 2020-03-27T17:45:04Z

web/src/components/SaveMessage.test.js

+      act(() => {
+        subject = create(<SaveMessage lastSaved={now} />);
+      })
+      expect(subject.toJSON()).toMatch("Saved");
+      act(() => jest.advanceTimersByTime(60 * 1000));
+      expect(subject.toJSON()).toMatch(/\(1 minute ago\)$/);


Totally not a hack. I think this is how React recommends doing tests if you've got hooks, really. It's possible we should being transitioning entirely to react-test-renderer. Otherwise, there are ways to make it work with Enzyme.

Again, something to put a pin in and revisit later. Definitely not something we have to figure out for this PR.

Addressed these concerns.

pphillips-fearless

I appreciate the extended conversations here as we come up to speed.

radavis · 2020-03-27T18:52:25Z

@jeromeleecms @lauraponce When you have a free moment, could you view the new save message functionality, here, please?: https://ec2-3-87-56-55.compute-1.amazonaws.com/apd

Thank you!

lauraponce

When I first load the page, it shows as "Saved" as if I just then saved it. I wasn't sure if this was just because of something with the test that we're using, or will it always save right when you load the page even when no new content is added?

mgwalker · 2020-03-28T00:23:42Z

@lauraponce Good catch! I noticed that too and remembered an issue from a while back where the APD immediately saves when it's loaded for the very first time. (The backend creates the APD and the frontend adds some initial data for the federal citations. Then the frontend tries to save it before anything gets lost.) It should only happen the first time an APD loads, so we need to double-check that. If it happens every time, that's probably a bug!

jeromeleecms

From what I can tell this works as intended. Thanks for the video! Now I don't have to wait for 1 year before I approve this PR 😂

Good to go on my end.

lauraponce

@mgwalker that makes sense! It must have just been the first time it was loading the APD (with the fake data in it). This looks good!

radavis · 2020-03-31T15:29:10Z

Regarding accessibility of this feature, from Heather Battaglia via Slack...

We'd had some discussions about autosave accessibility, and decided that announcing autosave every time it happened was too aggressive. We're announcing to screen readers when they land on the page that the page will be saved automatically, and then we're announcing errors when they happen, IIRC.

radavis added 3 commits March 24, 2020 18:41

add SaveMessage component to Header

2855caf

preserve space between Check and SaveMessage components

f9071f0

use moment#duration to calculate time differences, use expect#toMatch…

53f3b9b

… for better errors

radavis changed the title ~~2104 save message~~ Add SaveMessage component to Header Mar 25, 2020

radavis added 6 commits March 25, 2020 17:55

appease the demands of our linter

66aab17

thanks, yoda...

d8adad1

update Header test and snapshot

8f932be

working auto-update test

063f850

add dem semicolons

69e65a8

ugly auto-update test, but it seems to work

99421c4

radavis requested review from mgwalker and pphillips-fearless March 26, 2020 15:51

Merge branch 'master' into 2104-save-message

d91c94b

radavis commented Mar 26, 2020

View reviewed changes

refactor tests

ce556c7

pphillips-fearless previously requested changes Mar 26, 2020

View reviewed changes

remove leading zero from hour

4356bbd

mgwalker reviewed Mar 26, 2020

View reviewed changes

mgwalker approved these changes Mar 26, 2020

View reviewed changes

radavis requested review from jeromeleecms and lauraponce March 26, 2020 21:59

radavis requested review from mgwalker and pphillips-fearless March 26, 2020 22:00

radavis added 6 commits March 27, 2020 10:10

address feedback

022adc8

Merge branch 'master' into 2104-save-message

7f0ff45

refactor into functional component, auto-update test broken :(

b010620

use react-test-renderer for testing

017b419

update snapshots

f350adc

appease the linter

ea63be1

mgwalker approved these changes Mar 27, 2020

View reviewed changes

update snapshots

f27e9d5

pphillips-fearless approved these changes Mar 27, 2020

View reviewed changes

update changelog

d52a695

lauraponce reviewed Mar 27, 2020

View reviewed changes

jeromeleecms approved these changes Mar 30, 2020

View reviewed changes

lauraponce approved these changes Mar 30, 2020

View reviewed changes

lauraponce mentioned this pull request Mar 30, 2020

Update save message #2104

Closed

3 tasks

radavis merged commit 41fba39 into master Mar 31, 2020

radavis deleted the 2104-save-message branch March 31, 2020 17:57

		lastSaved = moment().subtract(seconds, "seconds");
		subject = shallow(

		const testName = `when saved ${value} ${timeUnit} ago, it displays "${result}"`
		test(testName, () => {

	const testName = `when saved ${value} ${timeUnit} ago, it displays "${result}"`
	test(testName, () => {
	test(`when saved ${value} ${timeUnit} ago, it displays "${result}"`, () => {

Add SaveMessage component to Header #2133

Add SaveMessage component to Header #2133

Conversation

radavis commented Mar 25, 2020 • edited

This pull request changes...

This pull request is ready to merge when...

This feature is done when...

What's missing/incongruent with Issue #2104...

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cms-eapd-bot commented Mar 26, 2020 • edited

pphillips-fearless left a comment

Choose a reason for hiding this comment

mgwalker commented Mar 26, 2020

jeromeleecms commented Mar 26, 2020

radavis commented Mar 26, 2020 • edited

radavis commented Mar 26, 2020 • edited

radavis commented Mar 26, 2020

jeromeleecms commented Mar 26, 2020

radavis commented Mar 26, 2020

jeromeleecms commented Mar 26, 2020

mgwalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radavis Mar 27, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgwalker Mar 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgwalker left a comment

Choose a reason for hiding this comment

radavis commented Mar 26, 2020

jjames521 commented Mar 26, 2020

mgwalker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pphillips-fearless left a comment

Choose a reason for hiding this comment

radavis commented Mar 27, 2020 • edited

lauraponce left a comment

Choose a reason for hiding this comment

mgwalker commented Mar 28, 2020

jeromeleecms left a comment

Choose a reason for hiding this comment

lauraponce left a comment

Choose a reason for hiding this comment

radavis commented Mar 31, 2020

radavis commented Mar 25, 2020 •

edited

cms-eapd-bot commented Mar 26, 2020 •

edited

radavis commented Mar 26, 2020 •

edited

radavis commented Mar 26, 2020 •

edited

radavis Mar 27, 2020 •

edited

mgwalker Mar 26, 2020 •

edited

radavis commented Mar 27, 2020 •

edited