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

Donut chart ux updates #1591

Merged
merged 1 commit into from
Sep 26, 2023
Merged

Donut chart ux updates #1591

merged 1 commit into from
Sep 26, 2023

Conversation

michaelnesen
Copy link
Collaborator

@michaelnesen michaelnesen commented Sep 25, 2023

What does this implement/fix?

After some UX discussion we have decided to implement the following changes:

  • Changed <DonutChart /> to include 0 or negative values in data.
  • Changed <DonutChart /> to show any number of data points.
  • Updated <DonutChart /> wrapper to take full height of container and center contents.

Slack thread for context on these changes

Does this close any currently open issues?

What do the changes look like?

Storybook link

http://localhost:6006/?path=/story/polaris-viz-charts-donutchart--default

Before merging

  • Check your changes on a variety of browsers and devices.

  • Update the Changelog's Unreleased section with your changes.

  • Update relevant documentation, tests, and Storybook.

  • Make sure you're exporting any new shared Components, Types and Utilities from the top level index file of the package

Comment on lines +50 to +51
const activeValueExists = activeValue !== null && activeValue !== undefined;
const valueToDisplay = activeValueExists ? activeValue : animatedTotalValue;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

activeValue can be 0 and we still want to show that in the center if thats the case.

@github-actions
Copy link

@github-actions
Copy link

github-actions bot commented Sep 25, 2023

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.3 KB (-0.05% 🔽) 1.3 s (-0.05% 🔽) 1.6 s (+9.29% 🔺) 2.8 s
polaris-viz-cjs 211.02 KB (-0.06% 🔽) 4.3 s (-0.06% 🔽) 3.5 s (+26.41% 🔺) 7.7 s
polaris-viz-esm 173.4 KB (-0.03% 🔽) 3.5 s (-0.03% 🔽) 1.5 s (-20.55% 🔽) 5 s
polaris-viz-css 4.57 KB (+0.13% 🔺) 92 ms (+0.13% 🔺) 482 ms (-0.44% 🔽) 573 ms
polaris-viz-esnext 178.51 KB (-0.03% 🔽) 3.6 s (-0.03% 🔽) 2.3 s (+69.4% 🔺) 5.9 s

Copy link
Member

@maryamkaka maryamkaka left a comment

Choose a reason for hiding this comment

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

🍩

@michaelnesen michaelnesen force-pushed the donut-chart-ux-updates branch 4 times, most recently from f789b55 to 0b744a4 Compare September 25, 2023 19:43
@michaelnesen michaelnesen merged commit 5b78114 into main Sep 26, 2023
6 checks passed
@shopify-shipit shopify-shipit bot temporarily deployed to production September 26, 2023 18:43 Inactive
@slack-github-archiver
Copy link

Summary

  • Claire, a FE developer, asked about a recent change to the donut chart used in their Tactic Report and how to handle the display when a legend item has a value of 0. She noticed that 0 values were recently filtered out.
  • Michael confirmed the recent change and initially suggested that Claire might need to update her version of Polaris-viz. However, he later realized that Claire was already using the latest version.
  • The issue seemed to be related to the use of a custom legend, which allows the consumer to decide what to show. Michael suggested that Claire would need to filter out the 0 value in the custom legend.
  • There was a discussion about the potential confusion of only showing the legend item for Returning customers and not for First-time customers when the latter has a value of 0.
  • Rachel suggested that negative and zero values should be filtered out in the visualization but should still be shown in the legend.
  • Michael agreed to make this change in Polaris-viz and released a new version. Claire confirmed she would update her version to incorporate these changes.

This summary was generated using OpenAI's gpt-4 with a temperature of 0.5.

🧵 Slack Thread
User Message
Claire DeBoer
2023‑09‑20 18:26
Hi team! I'm a FE dev on the Cube Measurement team and I'm wondering if someone could walk me through a recent change to the donut chart which we're using here in our Tactic Report and what should be shown in the center of the chart when a merchant hovers over a legend item that has a value of 0? I was working on a fix for showing 0 in the center of the donut when you hover over a legend item that has a 0 value (prod currently shows the total) but I see that 0 values were recently filtered out. Thanks very much! @michaelnesen
Screenshot 2023-09-20 at 2.21.09 PM.png
Michael Nesen
2023‑09‑20 18:46
Hey @clairedeboer :wave-frog: that’s correct we recently shipped a change in Polaris-viz which filtered out values <= 0 from the chart as they wouldn’t be shown in the visualization. However it looks like web doesn’t have the latest Polaris viz version yet so may need to bump to see those changes
Claire DeBoer
2023‑09‑20 18:49
Ahhh ok I'll start with that and report back, thank you!
Michael Nesen
2023‑09‑20 19:58
Hmm I just checked and it looks like web actually is on the latest version so you should have those changes :thunkface: Only values > 0 should appear in the legend. Do you mind pasting the data you are passing into the chart?
Michael Nesen
2023‑09‑20 20:00
Or rather, a link to the code for the custom legend rendered for this donut chart
Michael Nesen
2023‑09‑20 20:03
Thats probably why the 0 value is being shown, internally in p-viz we are filtering those out but when using a custom legend its up to the consumer what they want to show
Claire DeBoer
2023‑09‑20 20:04
Was just realizing that myself
Claire DeBoer
2023‑09‑20 20:04
Here is our donut chart with the custom legend https://github[…]FirstTimeVsReturningCustomers.tsx
Michael Nesen
2023‑09‑20 20:14
Nicee, yeah so since your using the custom legend override you’d have to filter out that data there 🙂
Claire DeBoer
2023‑09‑20 20:34
So the effect would be that in this case, we would only show the legend item for Returning customers? There would be no purple legend for First-time? That seems kinda confusing, but happy to go along if that is the pattern
Michael Nesen
2023‑09‑20 20:50
Hmm right, in this particular case it would be confusing since it’s referring to First time vs returning customers , and so just showing the one legend item would be odd :thinking_face:
Michael Nesen
2023‑09‑20 20:57
However internally that 0 value data point has been filtered out and so trying to reference it here with the activeValue on the renderInnerValueContent prop won’t work. So I see what you mean by what should be shown.
Michael Nesen
2023‑09‑20 21:04
Going to loop @rachelng in here to see if she has any thoughts about potentially only filtering out negative values, as this seems like a pretty good use case for wanting to show 0 values in the donut chart
Claire DeBoer
2023‑09‑20 21:06
Great, thanks so much for your help Michael!
Claire DeBoer
2023‑09‑20 21:07
cc @craigcolesshopify
Rachel Ng
2023‑09‑22 21:00
hi @clairedeboer! the idea is to filter out negative and zero values in the visualization but they should still be shown in the legend as we surface up to 5 items.

in your case, with the new legends + responsive view, it should look like this:


image.png
Michael Nesen
2023‑09‑22 21:04
I can work on making this change in Polaris-viz in the meantime and let your know when a release it out :blobthumbsup:
Claire DeBoer
2023‑09‑22 21:11
Got it that makes sense! So show 0 in the legend and also in the center of the donut on hover over a 0 legend item right?
Claire DeBoer
2023‑09‑22 21:12
@michaelnesen Would your fix do this or will there also be something I need to change once your release is out?
Michael Nesen
2023‑09‑22 21:16
The change in the library will take care of that :thumbs:
Michael Nesen
2023‑09‑26 19:09
Changes have shipped and new release is out: @shopify/[email protected]
Claire DeBoer
2023‑09‑26 19:12
Amazing!! Thanks so much for your help Michael! I'll go ahead and bump our version now

Claire DeBoer archived this conversation from optimize-analyze-team at 2023‑09‑26 19:19.
All times are in UTC.

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

Successfully merging this pull request may close these issues.

3 participants