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

[docs][base] Improve Base UI all components images #37590

Merged
merged 2 commits into from
Jun 21, 2023

Conversation

danilo-leal
Copy link
Contributor

A follow-up to #37536 to make images crispier. Also simplified the aspect ratio to a much more common one (16/9) and added explicit width and height sizes.

@danilo-leal danilo-leal added docs Improvements or additions to the documentation package: base-ui Specific to @mui/base labels Jun 14, 2023
@danilo-leal danilo-leal changed the title [docs] Improve Base UI all components images [docs][base] Improve Base UI all components images Jun 14, 2023
@michaldudak
Copy link
Member

Even though the images have the width set explicitly in HTML, they are not rendered at this width:
image

This is caused by the CSS rule placed on the image that sets its width to 100% of the parent, which, in turn, is calculated based on the grid width and number of columns. We could explicitly set the card width to avoid ugly fractions.

@danilo-leal
Copy link
Contributor Author

@michaldudak So we'd add an explicit width in pixel on the parent Card? Uhm... is that the best approach? 🤔

@mui-bot
Copy link

mui-bot commented Jun 19, 2023

Netlify deploy preview

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

Bundle size report

No bundle size changes

Generated by 🚫 dangerJS against 67ec75f

@danilo-leal
Copy link
Contributor Author

@michaldudak & @siriwatknp mind giving this one a second pass, please? 🙌

@michaldudak
Copy link
Member

TL;DR: yeah, let's ship it ;)

@michaldudak So we'd add an explicit width in pixel on the parent Card? Uhm... is that the best approach?

Not necessarily. I've found out the root cause of fractional widths is the max-width of div.main-content. It's set to 105ch and on my setup, this resolves to 814.333px. The width of a column in three-column layout with 16px gap is 260.771px, and the fractions continue.
This could be solved by setting .main-content's max-width to something like 57.5rem, which by default is 800px. One column is equal to a nice, round 256px in this case.

Now comes the (not so) fun part: images within cards are slightly smaller because cards have a 1px border. As I learned today, a 1px border isn't always a 1px border. Because I use 150% display scaling, my browsers (Edge and Firefox) render the border with 0.6667px width. This is (I assume) to ensure it occupies exactly one physical pixel and is crisp.

image

This means we don't know the exact width of the rendered image because it depends on display scaling settings. So no matter what we do, it could be a little blurry for some users.

Considering all the above, I suppose we can live with it. The version after the recent changes is better than before, so I'm ok with keeping it.

@danilo-leal
Copy link
Contributor Author

danilo-leal commented Jun 20, 2023

@michaldudak Shoot, well, at least you & I definitely learned something while trying to optimize this 😅
Appreciate the breakdown and summary! 🙌

I do need an approval, though, to merge it! 😅

@danilo-leal danilo-leal merged commit a91427e into master Jun 21, 2023
24 checks passed
@danilo-leal danilo-leal deleted the improve-base-component-image branch June 21, 2023 07:35
@danilo-leal danilo-leal self-assigned this Jun 23, 2023
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 package: base-ui Specific to @mui/base
Projects
Status: Recently completed
Development

Successfully merging this pull request may close these issues.

None yet

4 participants