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
Changes from 10 commits
2855caf
f9071f0
53f3b9b
66aab17
d8adad1
8f932be
063f850
69e65a8
99421c4
d91c94b
ce556c7
4356bbd
022adc8
7f0ff45
b010620
017b419
f350adc
ea63be1
f27e9d5
d52a695
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 { | ||
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")}`; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these would be better as |
||
result = `${result} (${lastSavedMoment.fromNow()})`; | ||
|
||
return result; | ||
} | ||
} | ||
|
||
SaveMessage.propTypes = { | ||
lastSaved: PropTypes.oneOfType([ | ||
PropTypes.instanceOf(Date), | ||
PropTypes.instanceOf(moment), | ||
PropTypes.string | ||
].isRequred), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, your There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(), | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since (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.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
Original file line number | Diff line number | Diff line change | ||||||||
---|---|---|---|---|---|---|---|---|---|---|
@@ -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; | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I'd move these closer to the point-of-use, at which point they can be |
||||||||||
|
||||||||||
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( | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like using const whenever possible There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 (Which is all to say, I don't love this but I also think it's probably the best solution.) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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\)$/); | ||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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\)$/); | ||||||||||
}); | ||||||||||
}); | ||||||||||
}); | ||||||||||
}); |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
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
anduseState
). 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.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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