-
Notifications
You must be signed in to change notification settings - Fork 0
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
JessicaH- Add day number on CommonsOverview #48
Conversation
In this PR, I added tests for calcualteDays and moved this function into dateUtils. An extra parameter, currentDate, is passed in to the function so that it can be tested with mock current dates. |
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.
Might want to make the PR title clearer, but other than that, looks good.
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.
LGTM!
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.
LGTM... will test on staff-qa
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.
Looks like there are some failing tests after rebasing this on main
pc - demo mocking date for CommonsOverview test
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.
Looks good to me, I see the changes of moving day0 to day1
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.
LGTM!
This includes the code from my last PR for font change that has not been merged yet. Review only CommonsOverview.js and dayUtils.js
In this PR, I added a utility to calculate the number of days since the starting day. Previously, the commons.day is empty. The new function is called and assigned to this variable so that it displays the right day number in CommonsOverview. May be adding test cases and error messages for starting dates set the future, or maybe change so that admin cannot create a starting date beyond the current date.
Storybook
Screenshots
Before:
After: