Skip to content

[charts][docs] Fix Population pyramid demo #17987

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

Merged
merged 3 commits into from
May 26, 2025

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented May 24, 2025

A continuation of #17652 and #17980. https://mui.com/x/react-charts/bar-demo/#population-pyramid has a number of problems:

  1. I have never seen an age pyramid rendered in this order. This is the "right" way IMHO: https://en.wikipedia.org/wiki/Population_pyramid.
  2. It's hard to read on mobile:
SCR-20250525-brlp
  1. We render a h6 in the page, orphan from the other headers.

Preview: https://deploy-preview-17987--material-ui-x.netlify.app/x/react-charts/bar-demo/#population-pyramid


Off-topics: I don't understand this:

export const DEFAULT_MARGINS = {
top: 20,
bottom: 20,
left: 20,
right: 20,
};

I thought we would get 0 for all those values. I had to set margin={{ right: 0, left: 0 }} and then use CSS to set the margin that I wanted, with responsive values. Proposing this for the next major: #17988.

Copy link

github-actions bot commented May 24, 2025

Thanks for adding a type label to the PR! 👍

@oliviertassinari oliviertassinari added type: bug A bug or unintended behavior in the components. docs Improvements or additions to the documentation scope: charts Changes or issues related to the charts product labels May 24, 2025
@oliviertassinari oliviertassinari marked this pull request as ready for review May 24, 2025 22:47
@mui-bot
Copy link

mui-bot commented May 24, 2025

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

Bundle size report

Total Size Change: 0B(0.00%) - Total Gzip Change: 0B(0.00%)
Files: 118 total (0 added, 0 removed, 0 changed)

Details of bundle changes

Generated by 🚫 dangerJS against b756b3d

Copy link

codspeed-hq bot commented May 24, 2025

CodSpeed Performance Report

Merging #17987 will not alter performance

Comparing oliviertassinari:fix-pyramid (b756b3d) with master (ea53ddc)

Summary

✅ 9 untouched benchmarks

@oliviertassinari oliviertassinari force-pushed the fix-pyramid branch 2 times, most recently from 26234b6 to c34fc15 Compare May 24, 2025 23:27
Copy link
Member

@bernardobelchior bernardobelchior left a comment

Choose a reason for hiding this comment

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

Good improvements, thank you 💪

@prakhargupta1
Copy link
Member

My two cents:
Maybe it should be called Horizontal diverging bar chart, like: https://nivo.rocks/storybook/?path=/story/bar--diverging-grouped

@bernardobelchior
Copy link
Member

Maybe it should be called Horizontal diverging bar chart

That feels like the technical name, but I suspect users will search for "Population pyramid" instead. I didn't even know this was called "horizontal diverging bar chart", so I think users won't either.

@oliviertassinari
Copy link
Member Author

oliviertassinari commented May 26, 2025

For the diverging bar chart, it feels like the pyramid charts is a too simple version of it. Looking at https://www.datylon.com/resources/chart-library/diverging-bar-chart, we might want something equivalent.

Now, I'm surprised that I can't find a demo of this in https://echarts.apache.org/examples/en/index.html#chart-type-bar or https://www.highcharts.com/demo#highcharts-demo-column-and-bar-charts.
But in here, it's called stacked horizontal: https://www.ag-grid.com/charts/gallery/stacked-horizontal-bar/.

In https://www.highcharts.com/demo/highcharts/bar-negative-stack, they give this a simpler name, so for #16587 we would likely need to invest into the search experience.

@prakhargupta1
Copy link
Member

In https://www.highcharts.com/demo/highcharts/bar-negative-stack, they give this a simpler name

Agree, Bar with negative values seems like a simpler name. Another example: https://apexcharts.com/javascript-chart-demos/bar-charts/bar-with-negative-values/

@alexfauquette
Copy link
Member

I did a few update:

  • I've replaced the default x-axis extremums by one that rounds at 100k population, and made sure the x-axis is symmetric. This is more realistic, plus it same some space on mobile
  • Added vertical grid for the ease of reading
  • I fixed the formatting that was not the same on male/female values in the tooltip.

Agree, Bar with negative values seems like a simpler name

I would prefer to keep the "Population pyramid" name since it's the official name of this chart.

We already have something named positiveandnegativebarchart taken from this Rechart example

We could add an extra demo similar to the diverging example you shared. If you agree we can add it in #17980

@oliviertassinari oliviertassinari merged commit ba45618 into mui:master May 26, 2025
22 checks passed
@oliviertassinari oliviertassinari deleted the fix-pyramid branch May 26, 2025 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation scope: charts Changes or issues related to the charts product type: bug A bug or unintended behavior in the components.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants