-
-
Notifications
You must be signed in to change notification settings - Fork 72
Yiyun add one more tab for weekly summaries #707
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
Yiyun add one more tab for weekly summaries #707
Conversation
|
@EvianTan I tested and confirmed the additional tab on weekly summaries. Nicely done! |
|
HI @EvianTan I tested the PR, I have one questions and one observation. 2 - I am getting an error in the console. Can this be fixed? 3- after save the three weeks ago summary the page is not clickable, the it is necessary to refresh thhe page. |
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.
Thank you for working hard on this PR with adding tab to the weekly summaries.
note: I am also getting the same error in with weeklySummary.jsx regarding uncaught promise.
Lucas-E
left a comment
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.
kangjay88
left a comment
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.
Hi @EvianTan, I have tested this PR and the '3 Weeks Ago Summary' does save and show. However, I am also not able to click anything else after submitting. This is also the case when I created a summary for 'last week's summary' and 2 weeks ago summary, unless I reload the page - maybe in a separate PR, code should implement a reload after saving?
HGN.APP.-.Google.Chrome.2023-03-11.10-53-53.mp4
|
Hey @EvianTan! I tested and it is working as expected. I also was not able to click the page after submitting the summary, but I tested the development branch and the error and the click behavior were already present. Maybe we could open a PR to solve this problem. Before Yiyun-add_one_more_tab_for_weekly_summaries_1.MP4After Yiyun-add_one_more_tab_for_weekly_summaries.MP4 |
PrabhjotSembhi
left a comment
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.
it's working fine.
here are some things that need to be addressed.
- after submitting a summary for any week page become unresponsive. You have reloaded it.
- We don't have 4 tabs for 4 weeks in Summary Reports
- Our tab naming is not consistent.
https://user-images.githubusercontent.com/82662768/225733624-5bfb8d5d-b979-421f-9a49-9f1b88471dee.mp4
check at 44 seconds it becomes unresponsive and it's still scrollable but I cant click anything.
You can here we have 3 tabs only and Plays makes names consistent which will make it easy to monitor.
|
Hi @EvianTan, |







Description
This PR is for URGENT bug #1
This PR is ONLY for adding one more tab on weekly summaries dashboard, to show the weekly summary of "three weeks ago".
This PR doesn't have any contribution on fixing the "total submitted".
Related PRS (if any):
#622 Alan worked on this before but didn't get it completed. I'm going to close it since I'll raise new ones.
Mainly changes explained:
src/components/WeeklySummary/WeeklySummary.jsx: update the UI and properties to make the weekly summaries dashboard show 4 tabs instead of 3, adddueDateThreeWeeksAgo,summaryThreeWeeksAgo, etc.src/components/WeeklySummary/WeeklySummary.test.jsx: update test suites to pass therun npm testcommand.How to test:
check into current branch
do
npm installand...to run this PR locallylog as any role user or a new created user
go to dashboard→ weekly summaries
verify adding summaries on the 4 tabs can work
Screenshots or videos of changes:
Note:
I made some changes on the Dev database for testing this PR

Before my change, weeklySummaries in Dev database somehow showed the three dates identical, both of them are the current week's due date. This link shows how the identical due dates problem found.
I used below commands to update the due dates to be correct. Since the due dates in our Beta database are correct, I didn't make any changes there.
From the below discussion, we decide to improve the current logic of counting "total submitted", I'll try to fix this part next week, in my next PR.
