-
-
Notifications
You must be signed in to change notification settings - Fork 380
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
feat: display date as time has passed #3840
Conversation
Nice one so far @onim-at - looks like exactly the right direction. 🥳 |
13c0831
to
008f1ed
Compare
onearmy-community-platform Run #6226
Run Properties:
|
Project |
onearmy-community-platform
|
Branch Review |
pull/3840
|
Run status |
Passed #6226
|
Run duration | 04m 34s |
Commit |
25af3a3861: test: add unit test to DisplayDate component
|
Committer | cosimochetta |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
2
|
Pending |
0
|
Skipped |
0
|
Passing |
60
|
View all changes introduced in this branch ↗︎ |
Hello @benfurber! I've added a check on the date in the question cypress test, is this what you intended? |
@onim-at Nice one, thank you. I'll have a look today. You should be able to join discord with this link. |
Visit the preview URL for this PR (updated for commit 25af3a3): https://onearmy-next--pr3840-3653-date-kffnhths.web.app (expires Sun, 06 Oct 2024 15:30:48 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: 6d65e4f8fee2f6ab2da0c1c3b85b8797d66afa59 |
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.
Just the one thing that's an absolute must change with the console log but there's a couple of bits I'd appreciate.
If you have a look at DiscussionTitle
you'll see a style of stories/tests I think would be good.
The stories file has a separate function for each display expected (roughly) and then a test checks that each function has the expected output. So in this instance roughly somthing like:
stories:
export const TwoMonthsAGo: StoryFn<typeof DisplayDate> = () => {
const twoMonthsAGo = subMonths(new Date(), 2)
return (
<DisplayDate date={twoMonthsAGo}></DisplayDate>
)}
test:
describe('DisplayDate', () => {
...
it('renders correctly when two months ago', () => {
const { getByText } = render(
<TwoMonthsAGo {...(TwoMonthsAGo.args as IProps)} />,
)
expect(getByText("2 months ago")).toBeInTheDocument()
})
Then it's a quick test of our implementation of the package (not the package itself) and more importantly, prevents a regression.
🎉 This PR is included in version 1.216.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Looks absolutely awesome! I'm really happy to see it implemented! Great job @onim-at and @benfurber! 🎉 |
PR Checklist
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Dates are shown as timestamp
What is the new behavior?
Dates are shown as "time has passed"
To-Do
List everywhere it should go:
Does this PR introduce a breaking change?
Git Issues
Closes #3653
What happens next?
Thanks for the contribution! We try to make sure all PRs are reviewed ahead of our monthly maintainers call (first Monday of the month)
If the PR is working as intended it'll be merged and included in the next platform release, if not changes will be requested and re-reviewed once updated.
If you need more immediate feedback you can try reaching out on Discord in the Community Platform
development
channel.