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 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion web/src/components/Header.js
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
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
@@ -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;
145 changes: 145 additions & 0 deletions web/src/components/SaveMessage.test.js
@@ -0,0 +1,145 @@
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', () => {
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();
});

describe("when saved 1 minute ago", () => {
it('displays "Last saved 11:59 am (1 minute ago)"', () => {
lastSaved = moment(jan1AtNoon).subtract(1, "minutes");
subject = shallow(
<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toMatch(/^Last saved/);
expect(subject.text()).toMatch(/11:59 am/);
expect(subject.text()).toMatch(/\(1 minute ago\)$/);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 I like them separate

});
});

describe("when saved 23 hours and 59 minutes ago", () => {
it('displays "Last saved 12:01 pm (1 day ago)"', () => {
lastSaved = moment(jan1AtNoon)
.subtract(23, "hours")
.subtract(59, "minutes");
subject = shallow(
<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toMatch(/^Last saved/);
expect(subject.text()).toMatch(/12:01 pm/);
expect(subject.text()).toMatch(/\(1 day ago\)$/);
});
});

describe("when saved 1 day ago", () => {
it('displays "Last saved December 30 (1 day ago)"', () => {
lastSaved = moment().subtract(1, "day");
subject = shallow(
<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toMatch(/^Last saved/);
expect(subject.text()).toMatch(/December 31/);
expect(subject.text()).toMatch(/\(1 day ago\)$/);
});
});

describe("when saved 30 days ago", () => {
it('displays "Last saved December 2 (1 month ago)"', () => {
lastSaved = moment().subtract(30, "day");
subject = shallow(
<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toMatch(/^Last saved/);
expect(subject.text()).toMatch(/December 2/);
expect(subject.text()).not.toMatch(/2019/);
expect(subject.text()).toMatch(/\(1 month ago\)$/);
});
});

describe("when saved 364 days ago", () => {
it('displays "Last saved January 2 (1 year ago)"', () => {
lastSaved = moment().subtract(364, "day");
subject = shallow(
<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toMatch(/^Last saved/);
expect(subject.text()).toMatch(/January 2/);
expect(subject.text()).not.toMatch(/2019/);
expect(subject.text()).toMatch(/\(1 year ago\)$/);
});
});

describe("when saved 3 years ago", () => {
it('displays "Last saved January 1, 2017 (3 years ago)"', () => {
lastSaved = moment().subtract(3, "years");
subject = shallow(
<SaveMessage lastSaved={lastSaved} />
);
expect(subject.text()).toMatch(/^Last saved/);
expect(subject.text()).toMatch(/January 1, 2017/);
expect(subject.text()).toMatch(/\(3 years ago\)$/);
});
});
});
});
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.