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

Jay fix color discrepancy for pie chart in PeopleReport #2326

Merged
merged 8 commits into from
Aug 24, 2024

Conversation

jayyy-s
Copy link
Contributor

@jayyy-s jayyy-s commented Jun 6, 2024

Description

Fixes color discrepancy for pie charts on people reports (medium priority bug)

image

Related PRS (if any):

Use the latest development branch of HGNRest

Main changes explained:

  • Edit /components/Reports/PeopleReport/components/PeopleTasksPieChart.jsx so only the dataLegend prop changes with View All toggle
  • Edit /components/common/PieChart/PieChart.jsx so there's a separate prop for chart labels called chartLegend

How to test:

  1. check into current branch
  2. do npm install and npm i to run this PR locally
  3. Clear site data/cache
  4. log as owner/admin user
  5. go to dashboard→ Reports dropdown→ Reports→People→type Ilya and check Ilya Volunteer (can check any other user that has tasks completed pie chart)
  6. Verify that the colors on the pie chart match when toggling all and do not change when toggling between viewing all tasks and collapsing

Screenshots or videos of changes:

PR.video.mp4

Kavil-Jain-514
Kavil-Jain-514 previously approved these changes Jun 6, 2024
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 functionality works perfectly fine.
The UI looks beautiful!!
image
image

I have a suggestion: in dark mode, the addYourProfilePic is not readable.
This is not related to PR but a suggestion if the image could be replaced with a better one where the text is visible
image

tianyangL11
tianyangL11 previously approved these changes Jun 8, 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.

ed9292cade68f587cd296d7788c3c7c
The PR meets the requirements. The colors on the pie chart remain consistent when toggling between viewing all tasks and collapsing. Everything functions as expected. Great job!

Copy link
Contributor

@shradac shradac left a comment

Choose a reason for hiding this comment

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

It matches when we collapse and expand, but noticed a discrepancy in the color in pie chart and tag for it. Check "Eat a pie 2", name tag is green and color on pie chart is pink.

week1-11

@beblicarl beblicarl self-requested a review June 14, 2024 07:09
Copy link
Contributor

@beblicarl beblicarl left a comment

Choose a reason for hiding this comment

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

The colours did not match example "Eat a pie 2", "Eat a pie 3" and "Eat a pie 4"

screencast-localhost_3000-2024.06.14-07_20_27.webm

Copy link
Contributor

@aaryaneil aaryaneil left a comment

Choose a reason for hiding this comment

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

Some of the colors don't match on the pie chart. I've added a few to the screenshots, please look into it.
Screenshot 2024-06-15 at 12 53 25 AM
Screenshot 2024-06-15 at 12 54 56 AM
Screenshot 2024-06-15 at 12 55 04 AM

@jayyy-s jayyy-s dismissed stale reviews from tianyangL11 and Kavil-Jain-514 via 904dc72 June 20, 2024 02:05
@jayyy-s jayyy-s requested review from aaryaneil and beblicarl June 20, 2024 02:09
@jayyy-s
Copy link
Contributor Author

jayyy-s commented Jun 20, 2024

Some of the colors don't match on the pie chart. I've added a few to the screenshots, please look into it. Screenshot 2024-06-15 at 12 53 25 AM Screenshot 2024-06-15 at 12 54 56 AM Screenshot 2024-06-15 at 12 55 04 AM

The colours did not match example "Eat a pie 2", "Eat a pie 3" and "Eat a pie 4"
screencast-localhost_3000-2024.06.14-07_20_27.webm

This issue has been addressed in the latest commit and should now be fixed. Thank you for the thorough review + feedback!

@one-community one-community added the High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible label Jul 18, 2024
@kabraambika kabraambika self-requested a review July 19, 2024 21:27
kabraambika
kabraambika previously approved these changes Jul 19, 2024
Copy link
Contributor

@kabraambika kabraambika left a comment

Choose a reason for hiding this comment

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

I reviewed the frontend PR with the latest backend on the development branch. The colors are displaying correctly in both dark and light modes. After using the view all and collapse functions, the colors match the associated legend as shown in the video.

PR2326.mov

ishan-miglani
ishan-miglani previously approved these changes Jul 19, 2024
Copy link
Contributor

@ishan-miglani ishan-miglani 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 have verified the changes in this PR. the functionality works correctly. The colors on the pie chart match when toggling all and do not change when toggling between viewing all tasks and collapsing. Thanks!

Screen.Recording.2024-07-19.at.5.52.04.PM.mov

Copy link
Contributor

@SrikanthChowdary495 SrikanthChowdary495 left a comment

Choose a reason for hiding this comment

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

The PR Working as expected. The colors on the pie chart match when toggling all and do not change when toggling between viewing all tasks and collapsing.
color

Copy link
Contributor

@angelalalacheng angelalalacheng 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 use your branch Jay_fix_color_discrepancy_pie_chart and the latest develpent branch for backend. It all works good in both light and dark mode.

Screenshot 2024-07-21 at 5 12 09 PM
Screenshot 2024-07-21 at 5 12 45 PM
Screenshot 2024-07-21 at 5 13 02 PM
Screenshot 2024-07-21 at 5 12 54 PM

Copy link
Contributor

@Pranay-ai Pranay-ai left a comment

Choose a reason for hiding this comment

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

Hey @jayyy-s
I have reviewed the PR , the colors match fine with list items but i found some mis alignment in the components the hours column is missing in desktop view
image

@souravwalke
Copy link

Hi, I tested the changes in the PR and It all works as expected.

Screenshot 2024-07-27 at 11 26 19 PM

ReinaT5678
ReinaT5678 previously approved these changes Jul 30, 2024
Copy link
Contributor

@ReinaT5678 ReinaT5678 left a comment

Choose a reason for hiding this comment

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

Looking at IIlya Volunteer and all the colors match the ones on the pie chart! I also played around with light and dark mode and changing between tasks and the colors did not change.

Screenshot 2024-07-29 at 9 29 16 PM Screenshot 2024-07-29 at 9 29 30 PM

SatyashanthiT
SatyashanthiT previously approved these changes Aug 3, 2024
Copy link
Contributor

@SatyashanthiT SatyashanthiT left a comment

Choose a reason for hiding this comment

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

I have tested this PR following the test instructions and have observed the features are working correctly
color img1
color img2

varunreddy03
varunreddy03 previously approved these changes Aug 4, 2024
Copy link

@varunreddy03 varunreddy03 left a comment

Choose a reason for hiding this comment

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

I have verified this PR and viewed the report dashboard and the pie charts, the colors are consistent across both the light and dark mode. Great work with the UI and colors they are well suited for both modes

PR 2326_1
PR 2326_2

Copy link
Contributor

@adityasure9 adityasure9 left a comment

Choose a reason for hiding this comment

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

Tested the PR and verified that the colors on the pie chart are consistent across both light and dark modes. The functionality works as expected when toggling between viewing all tasks and collapsing. Great work on maintaining the UI consistency.

Before,
1

After,
2

@jayyy-s jayyy-s dismissed stale reviews from varunreddy03 and SatyashanthiT via 6dc63d5 August 14, 2024 03:26
@jayyy-s jayyy-s requested a review from Pranay-ai August 14, 2024 03:41
@jayyy-s
Copy link
Contributor Author

jayyy-s commented Aug 14, 2024

Hey @jayyy-s I have reviewed the PR , the colors match fine with list items but i found some mis alignment in the components the hours column is missing in desktop view image

I've addressed this in the latest commits. This wasn't in the original scope of the bug, but I don't think it's worth creating a new issue for it. Thanks for catching it so we could fix it in this PR.

Copy link
Contributor

@Lemonnycodes Lemonnycodes 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 have tested the changes introduced in this PR:

  • Verified that the pie chart colors for tasks completed by users (e.g., Ilya Volunteer) remain consistent when toggling between viewing all tasks and collapsing the view.
  • The colors on the pie chart match and do not change during toggling, as expected.
  • The feature works correctly in both light and dark modes

Screenshots:
PR 2326 (1)
PR 2326 (2)
PR 2326 (3)

@one-community one-community added Moved to Final Review and removed High Priority - Please Review First This is an important PR we'd like to get merged as soon as possible labels Aug 23, 2024
@one-community
Copy link
Member

Thank you all, moving this to final review!

@beblicarl
Copy link
Contributor

the colors match perfectly now. This functionality works as intended. Great work

image

@one-community one-community merged commit 0ef795c into development Aug 24, 2024
1 check passed
@one-community
Copy link
Member

Thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.