-
Notifications
You must be signed in to change notification settings - Fork 400
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: Menu jumps vertically on logout #3194
Conversation
@muffinresearch, |
This seems fine, I'm just confirming it doesn't change anything on other screen sizes... |
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.
(that's responsive mode in Firefox at 320 x 480, btw) |
Codecov Report
@@ Coverage Diff @@
## master mozilla/addons-frontend#3194 +/- ##
==========================================
+ Coverage 95.23% 95.24% +0.01%
==========================================
Files 158 158
Lines 2916 2924 +8
Branches 575 577 +2
==========================================
+ Hits 2777 2785 +8
Misses 120 120
Partials 19 19
Continue to review full report at Codecov.
|
@tofumatt , I got confused between screen size variables then I got |
|
padding-bottom: 6px; | ||
padding-top: 8px; | ||
padding-bottom: 2px; | ||
padding-top: 4px; |
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 should have left a comment obviously. The padding-bottom
was important to avoid the menu to auto-close itself (because of focus lost) when user moves the cursor from username to the menu itself.
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 tested locally and it is all good.
padding-bottom: 6px; | ||
padding-top: 8px; | ||
padding-bottom: 2px; | ||
padding-top: 4px; |
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 still observe a difference between the menu and login button. It seems there is a 0.25px
difference... Could you set the height of the DropdownMenu
to 27px
?
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.
@willdurand , I was looking at desktop version where height: 27px
had no effect :(
Then I saw the mobile version :P , I forgot #mobileFirst
Here you go -
Mobile:
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.
Yeah, sorry for that. There is actually an issue on desktop now :-(
@tsl143 sorry, can you try adding |
@willdurand Instead, how about adding |
if that works, why not. I tested locally with |
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 tested it locally, and it works for me 👍
@willdurand has confirmed that the issue in #3194 (review) is resolved. Thanks @tsl143 ! |
Fixes: mozilla/addons#10792
Fixed GIF