[MenuItem] Fix text vertical alignment in IE 11 #17332
Conversation
Details of bundle changes.Comparing: 6a1c305...653edfd
|
|
It looks like after the change the element has minWidth of 48, but then adds padding top and bottom of 8 from ListItem. It makes menu item look huge (64px high). I recommend adding |
|
@damir-sirola I push changes with the best alternative I can think of, I'm not very happy with the customization impact of it, but I figure that people changing the height would be happy to have IE 11 support anyway.
I think that we need to keep the padding values for the dense support. |
oliviertassinari
changed the title
oliviertassinari
added
bug 🐛
|
@oliviertassinari I think this change will cause way more problems than it fixes. I've seen people (in StackOverflow questions) put all sorts of things in MenuItems and anything taller than a single line of text will now get vertically cut off. Before: https://codesandbox.io/s/wrapping-menuitem-z0utn |
|
@ryancogswell Do you have a recommendation on the used solution? It seems that we are not using the right typography variant: https://material.io/components/menus/#specs. I can confirm with most of the Google products, they use a font size of 14px in all the menus, on desktop. On mobile, they seem to use 16px. So I would propose the following fix a 2-in-1: diff --git a/packages/material-ui/src/MenuItem/MenuItem.js b/packages/material-ui/src/MenuItem/MenuItem.js
index f62a2cc9a5..a824e3b3aa 100644
--- a/packages/material-ui/src/MenuItem/MenuItem.js
+++ b/packages/material-ui/src/MenuItem/MenuItem.js
@@ -7,26 +7,27 @@ import ListItem from '../ListItem';
export const styles = theme => ({
/* Styles applied to the root element. */
root: {
- ...theme.typography.subtitle1,
+ ...theme.typography.body1,
minHeight: 48,
- paddingTop: 4,
- paddingBottom: 4,
+ paddingTop: 6,
+ paddingBottom: 6,
boxSizing: 'border-box',
width: 'auto',
overflow: 'hidden',
whiteSpace: 'nowrap',
+ [theme.breakpoints.up('sm')]: {
+ ...theme.typography.body2,
+ minHeight: 'auto',
+ }
},
+ // To remove in v5
/* Styles applied to the root element if `disableGutters={false}`. */
- gutters: {
- paddingLeft: 16,
- paddingRight: 16,
- },
+ gutters: {},
/* Styles applied to the root element if `selected={true}`. */
selected: {},
+ // To remove in v5
/* Styles applied to the root element if dense. */
- dense: {
- minHeight: 'auto',
- },
+ dense: {},
});small screens other screens |
oliviertassinari
added
the
design: material
| @@ -7,24 +7,27 @@ import ListItem from '../ListItem'; | |||
| export const styles = theme => ({ | |||
| /* Styles applied to the root element. */ | |||
| root: { | |||
| ...theme.typography.subtitle1, | |||
| ...theme.typography.body1, | |||
There was a problem hiding this comment.
This needs special changelog attention. It's likely this is considered a breaking change (using theme override to change MenuItem typography).
There was a problem hiding this comment.
The specification seems to encourage body2 but it contradicts all the implementatin of Google on mobile I can look at. I think that body1 is fine here. The intent is to use the default typography people use on the rest of the page.
There was a problem hiding this comment.
I think that body1 is fine here. The intent is to use the default typography people use on the rest of the page.
I was not talking about whether this is "fine" or "ok" or any other value judgment. Again a "breaking" change does not value the change. It simply states the implications a change has.
This change might have some which is why we should add a warning to the changelog. This is not concerned with judging this change on its merits.
There was a problem hiding this comment.
Agree, I was considering the impact of the change. The changes aim at following the spec, the breaking impact should be limited and subject to interpretation.
|
I expect this change to be controversial, the specification multiple contradictions around what the correct size should be: https://material.io/components/menus/#specs. While the pull request started as an effort to solve IE 11 support, it has grown to something with a bigger scope. So far, the main motivation of the change is to better match the design of Google products, I have benchmarked the changes with:
I hope the tradeoff is acceptable. We have height: 36px, font-size 16px. If further changes are required, we have a good week to do them. |
Closes #17323
Closes #17477