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

[Masonry] Improve the styling on the demos #27957

Merged
merged 2 commits into from
Sep 2, 2021

Conversation

hbjORbj
Copy link
Member

@hbjORbj hbjORbj commented Aug 25, 2021

The style of the div items used for demo of Masonry can be improved to be consistent with those used for demo of Grid, which can be found here: https://next.material-ui.com/components/grid/#main-content

Before:
https://deploy-preview-27439--material-ui.netlify.app/components/masonry/
Screenshot 2021-08-25 at 13 55 43

After:
https://deploy-preview-27957--material-ui.netlify.app/components/masonry/
Screenshot 2021-08-25 at 14 15 24

@hbjORbj hbjORbj added new feature New feature or request component: masonry This is the name of the generic UI component, not the React module! labels Aug 25, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Aug 25, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against a5bc49c

@mnajdova
Copy link
Member

I would propose maybe using bigger font for the numbers and putting it in the center of the box, but I would leave it to @danilo-leal :)

@hbjORbj hbjORbj requested a review from danilo-leal August 25, 2021 13:34
@mnajdova mnajdova changed the title [Masonry] Improve style of div items in demos (as same as those in Grid) [Masonry] Improve the styling on the demos Aug 25, 2021
@hbjORbj hbjORbj force-pushed the improve-masonry-demos branch from e5bae05 to bef2473 Compare August 25, 2021 15:01
@eps1lon
Copy link
Member

eps1lon commented Aug 27, 2021

I would personally not apply the new styles. The old ones were crystal clear. In the end, you likely don't need the styles in the final product anyway (considering the very next example).

@mnajdova
Copy link
Member

@eps1lon the idea was that it would look like the examples on https://next.material-ui.com/components/grid/#main-content

@eps1lon
Copy link
Member

eps1lon commented Aug 27, 2021

@eps1lon the idea was that it would look like the examples on next.material-ui.com/components/grid/#main-content

Ok, that's what the PR description already said. But why?

@mnajdova
Copy link
Member

mnajdova commented Aug 27, 2021

Ok. But why?

To look better :)

@eps1lon
Copy link
Member

eps1lon commented Aug 27, 2021

Ok. But why?

To looks better :)

The question is what makes these new styles better. You can't just claim that something "looks better".

@mnajdova
Copy link
Member

The question is what makes these new styles better. You can't just claim that something "looks better".

This is why I mention @danilo-leal to give an opinion on the matter. The idea for updating the styles was mine, but I will leave the last decision to @danilo-leal

Also, the code in the examples is much more readable now.

@eps1lon
Copy link
Member

eps1lon commented Aug 27, 2021

Could you share what the purpose of the new styles? Seems like you already told this Danilo (otherwise there's not really an oppinion to be had). But we need this info in the PR for future reference.

@mnajdova
Copy link
Member

Could you share what the purpose of the new styles?

My only thought was that we can make the demos look a bit better. We have done this in the past on the other examples where we use boxes for showing the component's usage:

If you strongly thing we should not do this, we can drop the effort, but I like these new styles better (it's my personal opinion)

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.

I'm in for this restyling :) It looks much better indeed and it is consistent with other components.

I would propose maybe using bigger font for the numbers and putting it in the center of the box

Regarding this @mnajdova's suggestion, I feel like the numbers font-size is alright but putting them in the center would probably be better indeed!

@hbjORbj
Copy link
Member Author

hbjORbj commented Aug 30, 2021

I'm in for this restyling :) It looks much better indeed and it is consistent with other components.

I would propose maybe using bigger font for the numbers and putting it in the center of the box

Regarding this @mnajdova's suggestion, I feel like the numbers font-size is alright but putting them in the center would probably be better indeed!

Thanks, I addressed your comment!

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Screenshot 2021-08-30 at 11 41 58

  • Regarding the new look and feel. I'm personally not a fan of the elevation, Google seems to be moving increasingly toward border outlined (on its new product). But the elevation looks great in this context, and it's sill heavily used by Google on its dashboards (e.g. firebase, google cloud console, google search console, google ads) so 👍

@eps1lon eps1lon removed their request for review August 30, 2021 11:43
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.

I believe that for now, we're good to go! About one of @oliviertassinari comments, this one:

The dark theme on https://next.material-ui.com/components/grid/ doesn't look awesome in my opinion. I struggle with the color contrast.

Although it's technically in the scope of "improving Masonry styling" he brings up the Grid as another example of a color contrast issue. Since there are these two and probably other components with this problem, I think we could probably tackle them all in a separate PR?! We could then close this one for now, not enlarging its scope (by fixing the Grid and other components here). And then open a new one for addressing the issue, as it is very worth to do so.

@mnajdova @hbjORbj what do you think?!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 1, 2021
@hbjORbj hbjORbj force-pushed the improve-masonry-demos branch from a9e94b8 to a5bc49c Compare September 2, 2021 10:32
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Sep 2, 2021
@hbjORbj
Copy link
Member Author

hbjORbj commented Sep 2, 2021

@danilo-leal I agree. I am merging it now!

@hbjORbj hbjORbj merged commit ff7362c into mui:next Sep 2, 2021
@hbjORbj hbjORbj mentioned this pull request Oct 22, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: masonry This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants