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

JessicaH- Add day number on CommonsOverview #48

Merged
merged 6 commits into from
Jun 6, 2023
Merged

Conversation

jessicahsy
Copy link
Contributor

@jessicahsy jessicahsy commented Jun 2, 2023

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:
Screen Shot 2023-06-01 at 3 05 09 PM

After:
Screen Shot 2023-06-01 at 3 05 40 PM

@jessicahsy jessicahsy marked this pull request as draft June 3, 2023 04:03
@jessicahsy
Copy link
Contributor Author

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.

@jessicahsy jessicahsy marked this pull request as ready for review June 3, 2023 06:27
Copy link
Contributor

@ykeung ykeung left a 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.

@pconrad pconrad added the FIXME-Needs peer CR Needs a code review from a team member before staff reviews it; remove this label after reviewing label Jun 3, 2023
@jessicahsy jessicahsy removed the FIXME-Needs peer CR Needs a code review from a team member before staff reviews it; remove this label after reviewing label Jun 4, 2023
ykeung
ykeung previously approved these changes Jun 4, 2023
Copy link
Contributor

@ykeung ykeung left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@pconrad pconrad left a 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

@pconrad pconrad added Merge Candidate Staff code review done; staff is testing on their own qa site before merging. and removed Merge Candidate Staff code review done; staff is testing on their own qa site before merging. labels Jun 4, 2023
Copy link
Contributor

@pconrad pconrad left a 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

@pconrad pconrad added the FIXME-Test failures There are test failures, but also advice on the PR and/or slack channel on how to fix them. label Jun 4, 2023
@jessicahsy jessicahsy requested a review from pconrad June 5, 2023 01:23
@jessicahsy jessicahsy changed the title Jessica h day num JessicaH- Add day number on CommonsOverview Jun 5, 2023
@jessicahsy jessicahsy removed the FIXME-Test failures There are test failures, but also advice on the PR and/or slack channel on how to fix them. label Jun 5, 2023
Copy link
Contributor

@kshitij0407 kshitij0407 left a 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

Copy link
Contributor

@pconrad pconrad left a comment

Choose a reason for hiding this comment

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

LGTM!

@pconrad pconrad merged commit 51a60da into main Jun 6, 2023
@pconrad pconrad added the 10 label Jun 6, 2023
@jessicahsy jessicahsy linked an issue Jun 8, 2023 that may be closed by this pull request
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: Duration Tracking - Improving UX for user commons page
4 participants