-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
fix mweb share collection dropdown #2627
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
fix mweb share collection dropdown #2627
Conversation
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 have a few thoughts here:
- You don't need the
position: absolute
because it's already absolute-positioned. - We're defining the
width
of the dropdown in terms of$base-font-size
so we should do the same here instead of using a hard-codedrem
value. left: -10rem
looks good down to about 360px but then goes off the left edge. If we switch fromleft
toright
based positioning then we can use amax-width
and get better support for very small screens. Maybemax-width: 90vw; right: #{120 / $base-font-size}rem;
?- This one is very opinionated and what you have for the breakpoint is not a problem. My opinion is that the breakpoint for something like this should be the breakpoint where this specific change becomes necessary, rather than applying it to all mobile screens. Personally I would put the breakpoint somewhere around
520px
. - I don't love that we have a style for the share button that depends on where that button appears in the layout. Like if we were to swap the two buttons then we would have to change the offset, which shows that the component is not fully encapsulated. But I'm not thinking of any better way to do it so I think we just deal with it not being "perfect" code.
@lindapaiste Thanks for feedback, really appreciate it. |
@lindapaiste Can you please review this ? Thanks |
Thanks for working on this! I think this could be a temporary solution for now until a more robust solution for addressing all of the dropdowns in the mobile UI is implemented! |
@raclim Sure, maybe i can work on developing more robust solution for addressing all of the |
Fixes #2624
Changes:
768px
I have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123