Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add SaveMessage component to Header #2133

Merged
merged 20 commits into from
Mar 31, 2020
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion web/src/components/Header.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { getIsAdmin } from '../reducers/user.selector';
import { t } from '../i18n';

import DashboardButton from './DashboardButton';
import SaveMessage from './SaveMessage';

import Icon, {
Check,
Expand Down Expand Up @@ -86,7 +87,7 @@ class Header extends Component {
</span>
) : (
<span>
<Check /> Saved {lastSaved}
<Check /> <SaveMessage lastSaved={lastSaved} />
</span>
)}
</span>
Expand Down
4 changes: 2 additions & 2 deletions web/src/components/Header.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ describe('Header component', () => {
}}
isAdmin={false}
isSaving={false}
lastSaved="last save date"
lastSaved="2020-01-01T12:00:00.000Z"
pushRoute={() => {}}
showSiteTitle={false}
/>
Expand All @@ -159,7 +159,7 @@ describe('Header component', () => {
}}
isAdmin={false}
isSaving
lastSaved="last save date"
lastSaved="2020-01-01T17:00:00.000Z"
pushRoute={() => {}}
showSiteTitle={false}
/>
Expand Down
87 changes: 87 additions & 0 deletions web/src/components/SaveMessage.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import moment from "moment";
import React from "react";
import PropTypes from 'prop-types';

// Configure moment to display '1 time-unit ago' instead of 'a time-unit ago'
// https://github.com/moment/moment/issues/3764
moment.updateLocale("en", {
relativeTime: {
s: "seconds",
m: "1 minute",
mm: "%d minutes",
h: "1 hour",
hh: "%d hours",
d: "1 day",
dd: "%d days",
M: "1 month",
MM: "%d months",
y: "1 year",
yy: "%d years",
},
});

class SaveMessage extends React.Component {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

constructor(props) {
super(props);

this.state = {
currentMoment: moment(),
};
}

componentDidMount() {
this.timerID = setInterval(() => this.updateClock(), 1000);
}

componentWillUnmount() {
clearInterval(this.timerID);
}

updateClock() {
this.setState({
currentMoment: moment(),
});
}

render() {
const { lastSaved } = this.props;
const { currentMoment } = this.state;

const lastSavedMoment = moment(lastSaved);
const difference = currentMoment.diff(lastSavedMoment);
const duration = moment.duration(difference);
let result = "Last saved";

if (duration.asMinutes() < 1) return "Saved";

// 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("hh:mm a")}`;
}
if (duration.asDays() >= 1 && duration.asYears() < 1) {
result = `${result} ${lastSavedMoment.format("MMMM D")}`;
}
if (duration.asYears() >= 1) {
result = `${result} ${lastSavedMoment.format("MMMM D, YYYY")}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

result = `${result} (${lastSavedMoment.fromNow()})`;

return result;
}
}

SaveMessage.propTypes = {
lastSaved: PropTypes.oneOfType([
PropTypes.instanceOf(Date),
PropTypes.instanceOf(moment),
PropTypes.string
].isRequred),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

};

SaveMessage.defaultProps = {
lastSaved: moment(),
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

@radavis radavis Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


export default SaveMessage;
87 changes: 87 additions & 0 deletions web/src/components/SaveMessage.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import React from "react";
import { shallow } from "enzyme";
import moment from "moment";
import SaveMessage from "./SaveMessage";

describe("<SaveMessage />", () => {
let lastSaved;
let subject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


describe('when saved less than 1 minute ago, it displays "Saved"', () => {
[
["1 second ago", 1],
["2 seconds ago", 2],
["30 seconds ago", 30],
["59 seconds", 59],
].forEach(([testName, seconds]) => {
test(testName, () => {
lastSaved = moment().subtract(seconds, "seconds");
subject = shallow(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 🤷‍♂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like using const whenever possible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toEqual("Saved");
});
});
});

describe("when observed saved time changes to 1 minute ago", () => {
const now = new Date(2020, 0, 1, 12, 0);
const oneMinuteFromNow = new Date(2020, 0, 1, 12, 1);
let mockDateNow;

beforeEach(() => {
jest.useFakeTimers();
mockDateNow = jest
.spyOn(Date, "now")
.mockReturnValueOnce(now)
.mockReturnValue(oneMinuteFromNow);
Copy link
Contributor

@mgwalker mgwalker Mar 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

afterEach(() => {
mockDateNow.mockRestore();
jest.clearAllTimers();
});

it('auto-updates from "Saved" to (1 minute ago)', () => {
subject = shallow(
<SaveMessage isSaving={false} lastSaved={now} />
);
expect(subject.text()).toMatch("Saved");
jest.advanceTimersByTime(1000);
expect(subject.text()).toMatch(/\(1 minute ago\)$/);
});
});

describe("given current time is January 1, 2020 12:00 pm", () => {
const jan1AtNoon = new Date(2020, 0, 1, 12, 0);
let mockDateNow;

beforeEach(() => {
mockDateNow = jest
.spyOn(Date, "now")
.mockReturnValue(jan1AtNoon.getTime());
});

afterEach(() => {
mockDateNow.mockRestore();
});

[
[1, "minute", "Last saved 11:59 am (1 minute ago)"],
[60 * 24 - 1, "minutes", "Last saved 12:01 pm (1 day ago)"],
[1, "day", "Last saved December 31 (1 day ago)"],
[30, "days", "Last saved December 2 (1 month ago)"],
[364, "days", "Last saved January 2 (1 year ago)"],
[3, "years", "Last saved January 1, 2017 (3 years ago)"],
].forEach(([value, timeUnit, result]) => {
let testName = `when saved ${value} ${timeUnit} ago, it displays "${result}"`
test(testName, () => {
lastSaved = moment().subtract(value, timeUnit);
subject = shallow(
<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toEqual(result);
});
});
});
});
6 changes: 4 additions & 2 deletions web/src/components/__snapshots__/Header.test.js.snap

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.