-
-
Notifications
You must be signed in to change notification settings - Fork 32.5k
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] Fix various layout issues with the v5 banner #27237
Conversation
mnajdova
commented
Jul 12, 2021
•
edited by oliviertassinari
Loading
edited by oliviertassinari
- Preview: https://deploy-preview-27237--material-ui.netlify.app/components/material-icons/#material-icons
- Preview: https://deploy-preview-27237--material-ui.netlify.app/company/careers/
Details of bundle changes.Comparing: 29e7f59...bac8fef Details of page changes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the height of the banner, 36px?
Yes, but it felt better with |
Ok, I didn't realize we were polishing the value too, makes sense. I asked because I have made a change in an opposite direction in 125f3db#diff-77f04bc58bbbb50bc7d3dea9ec533b08d76f110c581114cda8105da367d236a5R12 to fix an issue with the link anchor origin. I don't have a real preference on the approach as long as it's consistent. I'm unsubscribing to PR. Maybe in v5 we should use more constants, to make the math visible. It's never obvious if the value is purely from a design one or if it satisfies a layout constraint. |
Agree, let me in that case update to 36 so that we are going to be consistent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I don't spot any issue.
I have pushed the changes to the docs repo so they can go live within the next few minutes. |