Fix accounts menu styling #8707
Conversation
Builds ready [7e3e5af]
Page Load Metrics (638 ± 41 ms)
|
| flex-direction: column; | ||
| z-index: 200; | ||
|
|
||
| @media screen and (max-width: 575px) { |
There was a problem hiding this comment.
Why screen and max-width? This is a height issue. And why 575px? The popup has a height of 600px, but it looks like the menu is cut off until it gets above ~625px or so
| @media screen and (max-width: 575px) { | |
| @media (max-height: 625px) { |
There was a problem hiding this comment.
I just followed the other media queries in the file, all of which seem to be about handling the popup UI, and all of which use @media screen, and then max-width or min-width. See: https://github.com/MetaMask/metamask-extension/blob/develop/ui/app/components/app/account-menu/index.scss
There was a problem hiding this comment.
In fact, there are 177 existing media queries in our code base, and ~175 of them use @media screen and [max-|min-]width. If max-width, the value is (almost?) always 575px.
There was a problem hiding this comment.
I'm not sure what that has to do with this situation; I don't know what each of those are intended for. The other examples in this file of setting max-height in a max-width media query block also seem strange to me, but it's hard to say without knowing what they're intended to do.
There was a problem hiding this comment.
imo we'd target height here in the media query. It seems like having a discussion on media query formats and pulling those queries into some variables in SCSS would be a good idea in the long term.
There was a problem hiding this comment.
Very well; although I think we can pretty safely say that the purpose of those media queries is to apply styles to the popup UI, since their effect is to apply styles to the popup UI, and we'd expect to see such queries all over the codebase.
Whatever the case, we can address it later like Brad suggested.
Changed to max-height: 600px query in b98e0a9, since 600px is the height of the popup UI. It works.
There was a problem hiding this comment.
This is certainly an improvement, but it still leaves the Settings menu option obscured when the window is between 600px and 625px, as uncommon as those heights may be.
If we would prefer keeping the media queries restricted to certain screen sizes to simplify testing, we could pursue alternate solutions like setting a max-height based upon the viewport height.
There was a problem hiding this comment.
Ah, now I see what you mean. Sorry about that. I'll create an issue attempting to document these concerns. Ref: #8711
Builds ready [b98e0a9]
Page Load Metrics (642 ± 59 ms)
|
* origin/develop: (689 commits) Implement asset page (#8696) fix crash on signature request (#8709) Fix accounts menu styling (#8707) Delete docs/porting_to_new_environment.md (#8704) Remove unused `getToErrorObject` parameters (#8705) hide connected-status on metamask ext (#8703) Stop adding permissions middleware to trusted connections (#8701) Use `send` state for send flow token (#8695) do not display extension id in connection modal (#8699) Fix tab content disappearing during scrolling on macOS Firefox (#8702) close details when button is pressed (#8694) Refactor token selectors (#8671) Update eth_accounts permission description (#8693) Extract selected token from token input (#8692) Fix propType for Home defaultHomeActiveTabName (#8683) Fix create account form styling (#8689) Remove unused `getSelectedTokenAssetImage` selector (#8691) Remove `getTxParams` (#8676) do not show account mismatch alert on details (#8678) Fix connect hardware styling (#8680) ...
The accounts menu was getting clipped at the bottom in the popup under certain conditions. This has been fixed by the addition of a
max-heightproperty for the popup UI only.Before
After
Appearance with < 5 Accounts Unaffected