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

[website] Updating Charts demo with real charts usage for MUI X marketing page #38317

Merged
merged 2 commits into from
Aug 7, 2023

Conversation

richbustos
Copy link
Contributor

@richbustos richbustos commented Aug 4, 2023

Updating the Charts demo in MUI X landing page with real Charts usage. We were previously using images.

This is a continuation of #38027 and @alexfauquette work on #37609.

Preview: https://deploy-preview-38317--material-ui.netlify.app/x/

Before:
Screen Shot 2023-08-04 at 8 52 28 AM

After:
Screen Shot 2023-08-04 at 8 51 42 AM

@richbustos richbustos added the website Pages that are not documentation-related, marketing-focused. label Aug 4, 2023
@richbustos richbustos self-assigned this Aug 4, 2023
@samuelsycamore
Copy link
Contributor

This is awesome! Those color palettes really pop! 🤩

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Sweet, this is looking great!

Instead of stacking up two different types of charts, and making the demo container a bit too tall, what do you think of adding chips there so visitors can click on them to see each one? I know there's the downside of having to interact with it to see a different type of chart, but it'd certainly make the overall section/page more polished!

Screen Shot 2023-08-04 at 19 05 23

@richbustos
Copy link
Contributor Author

Instead of stacking up two different types of charts, and making the demo container a bit too tall, what do you think of adding chips there so visitors can click on them to see each one? I know there's the downside of having to interact with it to see a different type of chart, but it'd certainly make the overall section/page more polished!

Love the new designs! I can probably address that in a later PR? Possibly before a beta or stable release?

I wanted to get this one quickly out and not spend too much time on it. @danilo-leal

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 6, 2023

A quick look at it:

  1. Color intensity. Current:
Screenshot 2023-08-07 at 01 46 24

It feels like the colors could be lighter and the font size less bold, for example, this would feel better on my end (to give an idea).

Screenshot 2023-08-07 at 01 49 26
  1. Area opacity. It feels a bit strange with the area chart opacity to be full. Most of the other area charts I'm used to use some level of opacity. Why? I think to be able to see the axis, helping read the data. Examples:

So I suspect it would be better to change the default look in https://mui.com/x/react-charts/areas-demo/#simpleareachart cc @gerdadesign & @alexfauquette, I don't know if this is feedback that surfaced before. What do you think? I hear that for data engineering, it's also frequent to see the opposite https://matplotlib.org/stable/plot_types/basic/stackplot.html#sphx-glr-plot-types-basic-stackplot-py, https://vizzingdata.com/discrete-continuous-area-charts/.

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.

Thanks for pushing it

Added small refinements. Their is still an issue with tootlip opacity due to the docs theming
https://github.com/mui/material-ui/blob/a172994c2e15c91a1879897f84d4a711914e5f36/docs/src/modules/brandingTheme.ts/#L968-L969

About opacity, I don't know. From a DX point of view, I think it's easier to have opacity and let dev add an opacity property than having one encoded in the colors

@mui-bot
Copy link

mui-bot commented Aug 7, 2023

Netlify deploy preview

https://deploy-preview-38317--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 45556f5

@mnajdova
Copy link
Member

mnajdova commented Aug 7, 2023

Should I merge this before the release?

@richbustos
Copy link
Contributor Author

Should I merge this before the release?

We can skip this merge for the next release. I would like to wait for one more sign-off from @danilo-leal or @oliviertassinari.

Thanks!

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Sweet, it makes sense to iterate on a more complex demo later!

Just wondering if we shouldn't choose just one of the two charts to preserve the overall section's height ⎯ but aside from this, it's good to go, much better having the real component than illustrations!

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 7, 2023

From a DX point of view, I think it's easier to have opacity and let dev add an opacity property than having one encoded in the colors

@alexfauquette We could use plain rga color (and not rgba) combined with fill-opacity. But I think we can also make the case that there is less customization overall for developers to make if the charts are closer to their intended design. e.g. starting from the most popular Figma file for charts: https://www.figma.com/community/plugin/734590934750866002/Chart 80% opacity.


Color intensity.

What about this one?

@richbustos
Copy link
Contributor Author

Sweet, it makes sense to iterate on a more complex demo later!

Just wondering if we shouldn't choose just one of the two charts to preserve the overall section's height ⎯ but aside from this, it's good to go, much better having the real component than illustrations!

I dont think it looks too bad. I also looked at the other components and they seem to exceed the height as well.

Screen Shot 2023-08-07 at 9 26 00 AM
Screen Shot 2023-08-07 at 9 25 53 AM
Screen Shot 2023-08-07 at 9 25 47 AM

@danilo-leal
Copy link
Contributor

Yeah, all of them exceed it by a little, which is definitely not a problem ⎯ just that the Charts will exceed it more than I'd ideally like (the Date Range is already a bit too tall for me but doable). Not a blocker for this immediate release but looking forward to iterating more on it in the near future!

@richbustos richbustos merged commit 07aad34 into master Aug 7, 2023
@richbustos richbustos deleted the update-charts-demo branch August 7, 2023 20:25
richbustos added a commit that referenced this pull request Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts website Pages that are not documentation-related, marketing-focused.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants