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

Chris_spinning_wheel_date_change_reports #2298

Merged
merged 4 commits into from
Jun 29, 2024

Conversation

chrischenlixing
Copy link
Contributor

@chrischenlixing chrischenlixing commented May 30, 2024

Description

The issue with the original pr is that when the date is changed, there isn't an indicator to show that the data is retrieved or not. With the spinning wheels added, it's going to spin until the newest data is ready and graph is rerendered.

Related PRS (if any):

This PR is a minor patch for the functionality implemented in PR #2268.
No backend pr related, for the backend just check-in to the development branch

Main changes explained:

  • Added the state for "dataReady" and "setDataReady" for the spinning wheel of the total people report
  • Added the state for "dataReady" and "setDataReady" for the spinning wheel of the total project report

How to test:

  1. check into current branch
  2. do npm install and ... to run this PR locally
  3. Clear site data/cache
  4. log as admin user
  5. go to Reports →Reports → Projects/People/Teams
  6. Click on ‘Show Total Project Report’, ‘Show Total People Report’, and ‘Show Total Team Report’ one by one
  7. Change the end date to last year or any date you would want to test, see if there is a spinning wheel on there before the new report is shown for the new date

Screenshots or videos of changes:

Before, when changing the date for People Report and Project Report to a different date, the date would be refreshed to the new date but the new graph is lagged to render.
Screenshot 2024-05-29 at 11 58 00 PM
With the new spinning wheel, it's not going to show the graph until the new graph is ready with the updated date.
Screenshot 2024-05-29 at 11 42 30 PM

@Vijeth-V
Copy link

This works perfectly. The report would be displayed with the new date.

Screenshot 2024-05-30 at 11 01 08 PM

mohabbasd
mohabbasd previously approved these changes Jun 1, 2024
Copy link
Contributor

@mohabbasd mohabbasd left a comment

Choose a reason for hiding this comment

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

Hello, I tested you PR and I can see the spinning wheel, it's just that for some reason I am unable to access the database in this particular page other wise everything works. Below is a video for your reference.

Screen.Recording.2024-05-31.at.11.44.02.PM.mov

Copy link

@Sandhya1236 Sandhya1236 left a comment

Choose a reason for hiding this comment

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

While testing the recent updates that add spinning wheels to indicate data loading status when the date is changed, I encountered several issues. This PR is a minor patch for the functionality implemented in PR #2268, with no related backend PR. The main changes involved adding states for dataReady and setDataReady to handle the spinning wheels for the total people report, total project report, and total team report.

After checking into the current branch and running npm install, I cleared the site data and cache. I logged in as an admin user and navigated to Reports → Reports → Projects/People/Teams. I clicked on 'Show Total Project Report', 'Show Total People Report', and 'Show Total Team Report' one by one. When changing the end date to test different dates, the spinning wheel appeared as expected before the new report was shown.

However, I was unable to access the database on this particular page, which prevented me from fully verifying the functionality of the spinning wheel and data retrieval. Despite the spinning wheel displaying correctly, the inability to access the database means I couldn't confirm if the data was being retrieved and the graph was being re-rendered accurately. This issue needs to be resolved to ensure that the data loading indicator and data retrieval processes are functioning correctly.

HGN.APP.-.Google.Chrome.2024-05-31.21-57-24.mp4

@chrischenlixing
Copy link
Contributor Author

chrischenlixing commented Jun 1, 2024

While testing the recent updates that add spinning wheels to indicate data loading status when the date is changed, I encountered several issues. This PR is a minor patch for the functionality implemented in PR #2268, with no related backend PR. The main changes involved adding states for dataReady and setDataReady to handle the spinning wheels for the total people report, total project report, and total team report.

After checking into the current branch and running npm install, I cleared the site data and cache. I logged in as an admin user and navigated to Reports → Reports → Projects/People/Teams. I clicked on 'Show Total Project Report', 'Show Total People Report', and 'Show Total Team Report' one by one. When changing the end date to test different dates, the spinning wheel appeared as expected before the new report was shown.

However, I was unable to access the database on this particular page, which prevented me from fully verifying the functionality of the spinning wheel and data retrieval. Despite the spinning wheel displaying correctly, the inability to access the database means I couldn't confirm if the data was being retrieved and the graph was being re-rendered accurately. This issue needs to be resolved to ensure that the data loading indicator and data retrieval processes are functioning correctly.

HGN.APP.-.Google.Chrome.2024-05-31.21-57-24.mp4

Hi Sandhya, I tried to optimize some of the frontend rendering to make it load faster. Please test it again and be patient for it to render. Thanks!

peter6866
peter6866 previously approved these changes Jun 2, 2024
Copy link
Contributor

@peter6866 peter6866 left a comment

Choose a reason for hiding this comment

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

Great work on effectively addressing the lack of user feedback after changing report dates. The addition of loading spinners is a great improvement to the user experience.

https://www.loom.com/share/ba3cc185193d40d483fb8f2893f5bb9a?sid=e000794a-2567-49e9-ade4-20cb04291f17

tianyangL11
tianyangL11 previously approved these changes Jun 2, 2024
Copy link

@tianyangL11 tianyangL11 left a comment

Choose a reason for hiding this comment

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

It works perfectly. Please see the attached screenshots
4aa23e5280a4b9beac03d25b31672e8

@chrischenlixing chrischenlixing added the High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible label Jun 4, 2024
JatinAgrawal94
JatinAgrawal94 previously approved these changes Jun 5, 2024
Copy link
Contributor

@JatinAgrawal94 JatinAgrawal94 left a comment

Choose a reason for hiding this comment

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

Feature works as expected. Great Work!

PR.2298.mp4

@nishithaShetty
Copy link

Hi Chris, Nice work!
It works as expected. It functions properly for Projects, People, and Teams. During loading and when changing the dates for the given modules, the spinning wheel is rendered.

PR#2298_1 PR#2298_2 PR#2298_3 PR#2298_4

CloudyZ524
CloudyZ524 previously approved these changes Jun 6, 2024
Copy link

@CloudyZ524 CloudyZ524 left a comment

Choose a reason for hiding this comment

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

Hi Chris,
The spinning wheel updated in this pull request works well and the date were updated correctly when I reset the date ‘Show Total Project Report’, ‘Show Total People Report’, and ‘Show Total Team Report’ one by one. Good job!

Screenshot 2024-06-06 at 00 04 15

One suggestion about the content display is that I noticed the date format from the textbox of date selection and the date format on the report is different, which might cause some confusion to users. It would be more clear to unify the date format in the same page.

Screenshot 2024-06-06 at 00 03 15

@akshit0211
Copy link
Contributor

akshit0211 commented Jun 6, 2024

Hi Chris, great work for adding this spinning wheel functionality. It is working as expected for all three reports.

Spinning Wheel project report people report team report

akshit0211
akshit0211 previously approved these changes Jun 6, 2024
@Trip2310 Trip2310 requested review from Trip2310 and removed request for Trip2310 June 6, 2024 18:37
Wenboooooo
Wenboooooo previously approved these changes Jun 7, 2024
Copy link

@Wenboooooo Wenboooooo left a comment

Choose a reason for hiding this comment

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

Hello, I have tested the functionality using the admin account, the spinning wheel displayed as expected, great work!

2298.mp4

@PabloWang PabloWang self-requested a review June 7, 2024 15:55
PabloWang
PabloWang previously approved these changes Jun 7, 2024
Copy link

@PabloWang PabloWang left a comment

Choose a reason for hiding this comment

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

Hi Chris, good work! It works as expected

Screenshot 2024-06-07 at 11 45 38 AM Screenshot 2024-06-07 at 11 45 08 AM

Copy link
Contributor

@Sudershhh Sudershhh left a comment

Choose a reason for hiding this comment

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

Hi Chris,
The feature works as expected.
PR1

With this in mind, here are 2 more code changes that could be done to significantly enhance user experience.

  1. Renaming the dataReady state variable.
  2. Vertically aligning the loading spinner.

@@ -11,7 +11,7 @@ import Loading from '../../common/Loading';
function TotalPeopleReport(props) {
const { startDate, endDate, userProfiles, darkMode } = props;
const [dataLoading, setDataLoading] = useState(true);
const [dataRefresh, setDataRefresh] = useState(false);
const [dataReady, setDataReady] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it's better to name the state variable as specific as possible for better code readability and user experience.
For example, instead of dataReady , we could have totalPeopleReportDataReady or something along the lines of that. This would effectively convey the proper idea behind the state variable.

@@ -287,7 +242,7 @@ function TotalProjectReport(props) {

return (
<div>
{dataLoading ? (
{!dataReady ? (
<Loading align="center" darkMode={darkMode}/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The Loading animation can be centered Vertically around the mid area point of the div bar. It works correctly around the X-axis.

jennZhang93
jennZhang93 previously approved these changes Jun 8, 2024
Copy link
Contributor

@jennZhang93 jennZhang93 left a comment

Choose a reason for hiding this comment

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

Tested with owner account, I was able to see spinning wheels when graph isn't ready to load. One improvement suggestion is to alert user on the date range that the date selector can handle. Or block off the dates that shouldn't be selected. In the video below, I was trying to set the start date to 2004, but it keeps jumping back to 2014. I was confused for a minute until I realized that 2004 was too early.

newScreen.Recording.2024-06-07.at.7.47.49.PM.mov

Copy link
Contributor

@Sudershhh Sudershhh left a comment

Choose a reason for hiding this comment

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

Thanks for updating the requested changes. The new variables look good and more meaningful.

Copy link
Contributor

@Jingyii800 Jingyii800 left a comment

Choose a reason for hiding this comment

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

Hi Chris, I tested this feature with admin account. the wheel looks good while waiting for the chart generating.

bandicam.2024-06-12.15-22-41-640.mp4

Copy link

@TXMO-dev TXMO-dev left a comment

Choose a reason for hiding this comment

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

I checked out the branch, and it worked as desired. Everything functions correctly. Approved!

Copy link
Contributor

@suaniii suaniii left a comment

Choose a reason for hiding this comment

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

Hi, I've tested your PR, and it works well. I clicked into the reports page, then clicked on ‘Show Total Project Report’, ‘Show Total People Report’, and ‘Show Total Team Report’ one by one, and there was a spinning wheel displayed. Then, I changed the start date and end date, and the spinning wheels also displayed when the data was being retrieved. Great work!

2298.admin.mov

Copy link

@Sandhya1236 Sandhya1236 left a comment

Choose a reason for hiding this comment

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

The original PR had an issue where changing the date did not provide any indication if data retrieval was in progress or completed. This PR addresses the problem by adding spinning wheels to indicate data loading until the newest data is ready and the graph is re-rendered. It is a minor patch for the functionality implemented in PR #2268 and does not require any backend PR; for the backend, just check in to the development branch. The main changes include adding state variables dataReady and setDataReady for the spinning wheels in the total people and total project reports. To test, check into the current branch, run npm install and necessary commands to run this PR locally, clear site data/cache, log in as an admin user, navigate to Reports → Reports → Projects/People/Teams, and click on ‘Show Total Project Report’, ‘Show Total People Report’, and ‘Show Total Team Report’ one by one. Change the end date to last year or any other date you want to test, and verify if the spinning wheel appears before the new report is shown for the new date. The functionality was successfully tested, and everything is working fine.

HGN.APP.-.Google.Chrome.2024-06-13.23-20-42.mp4

@Trip2310 Trip2310 self-requested a review June 14, 2024 19:01
@Trip2310
Copy link

The functionality was tested by following the given steps. It works perfectly. Some screenshots attached
Screenshot 2024-06-14 132704
Screenshot 2024-06-14 132923
Screenshot 2024-06-14 132825

Copy link

@Trip2310 Trip2310 left a comment

Choose a reason for hiding this comment

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

I've tested this PR, and it works well. Following given testing steps one by one, and there is a spinning wheel displayed after that, I changed the date and year, and the spinning wheels displayed and shows all data smoothly. I approve
Working well

Copy link
Contributor

@Kavil-Jain-514 Kavil-Jain-514 left a comment

Choose a reason for hiding this comment

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

The spinning wheel appears every time there is a load time when displaying the reports and changing dates.
The PR works well.

PR.2298.mp4

@sahithidhulipala9
Copy link

Hi Chris, I have checked your PR and it is working perfectly. I have followed the steps you mentioned to check the functionality. Spinning wheel is working as it should be while loading the data. I have also checked changing the start and end dates and tried for all 3 i.e. for Show Total Reports, Hide Total People Report, Show Total Team Report.

https://www.loom.com/share/0e359bb9163f4869990bed9b7a1acba0?sid=e7b6018f-6f8c-4e03-96db-659e98302db2

Copy link
Contributor

@ImzIssa ImzIssa left a comment

Choose a reason for hiding this comment

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

The changes look good. Nice work.

Screenshot (405)

Copy link
Contributor

@vishavk1992 vishavk1992 left a comment

Choose a reason for hiding this comment

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

Hi chris , Tested this PR for for the spinning wheel of the total people report and project report. it works great and spinning wheel looks good.

PR-2298
PR--2298

@one-community
Copy link
Member

Thank you all, merging!

@one-community one-community merged commit 19d5a34 into development Jun 29, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible Moved to Final Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.