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

[charts] Use real world data for PieChart examples #14297

Merged
merged 7 commits into from
Sep 2, 2024

Conversation

JCQuintas
Copy link
Member

@JCQuintas JCQuintas commented Aug 22, 2024

Pie charts are quite simple charts that can't contain a lot of data, and our examples were geared towards that optimistic view.

Real world usage however, is more complicated.

I've tried using world population datasets (by continent and sub-regions, eg: Europe (continent), Balkans (sub-continent). But that created charts with too many small slices, due to what I guess is population concentration 🙃

My second try was internet browser usage, and landed on Platforms and Operating Systems (by platform) which provided a nice mix of very simple to slightly more complex data.

  • Using data from https://gs.statcounter.com
  • From December 2023
  • Data was manipulated in regards to condensing values that were too small into Other and removing Tablet from platforms

Data points

  • platforms: 2 data points
  • desktopOS: 5
  • mobileOS: 3 (only used in mobileAndDesktopOS)
  • mobileAndDesktopOS: 8

Preview

https://deploy-preview-14297--material-ui-x.netlify.app/x/react-charts/pie/

Side by Side comparison
Old New
screencapture-localhost-3001-x-react-charts-pie-2024-08-23-12_19_10 screencapture-localhost-3001-x-react-charts-pie-2024-08-23-12_17_57

@JCQuintas JCQuintas added docs Improvements or additions to the documentation component: charts This is the name of the generic UI component, not the React module! labels Aug 22, 2024
@JCQuintas JCQuintas self-assigned this Aug 22, 2024
@mui-bot
Copy link

mui-bot commented Aug 22, 2024

Deploy preview: https://deploy-preview-14297--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 20486ad

Copy link

codspeed-hq bot commented Aug 22, 2024

CodSpeed Performance Report

Merging #14297 will not alter performance

Comparing JCQuintas:use-real-world-pie-data (20486ad) with master (0c30a6f)

Summary

✅ 3 untouched benchmarks

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

I like the OS dataset. That's for sure something user will understand :)

docs/data/charts/pie/PieClickNoSnap.js Outdated Show resolved Hide resolved
docs/data/charts/pie/PieClickNoSnap.js Outdated Show resolved Hide resolved
JCQuintas and others added 2 commits August 26, 2024 16:44
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
Co-authored-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 27, 2024
Signed-off-by: Jose C Quintas Jr <juniorquintas@gmail.com>
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 27, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Aug 27, 2024
Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

Looks super nice. I enjoyed reading back this docs page with the new demo 👍

},
];

const normalize = (v: number, v2: number) => Number.parseFloat(((v * v2) / 100).toFixed(2));
Copy link
Member

Choose a reason for hiding this comment

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

Recently found that formatting method which simplify the management of digits after comma

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/NumberFormat/NumberFormat#style

new Intl.NumberFormat('de-DE', { style: 'percent', maximumFractionDigits: 2  }).format(
    number,
  ),

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think using the number format would be better in this case, we are clipping the number number so they are easier to read, yes, but they also need to be a numeric value, while Intl.NumberFormat would format it to a user to see, like adding . in the hundreds, eg: 1000 -> 1.000

Copy link
Member

@alexfauquette alexfauquette Sep 2, 2024

Choose a reason for hiding this comment

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

What triggered me is the .toFixed(2) before parsing.

You have do

  1. number computation (v * v2) / 100)
  2. transformation to string .toFixed(2)
  3. transformation to number Number.parseFloat()

Whereas you could

  1. have the correct numbers by doing only the (v * v2) / 100) in the normalize
  2. display value as expected with
const valueFormatter = new Intl.NumberFormat('de-DE', { style: 'percent', maximumFractionDigits: 2  }).format;

Intl.NumberFormat would format it to a user to see, like adding . in the hundreds, eg: 1000 -> 1.000

Yes, and with style: 'percent' it will add the percentage sygn, plus some space according to the locale, an manage rounding the values to get the correct number of digits, ...

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, that could be an options yes.

I don't think it matters much though, since we also have the data as 11.11 in the other dataset, this ensures all datasets have the same precision. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! docs Improvements or additions to the documentation size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants