Skip to content
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

Merged
merged 4 commits into from Sep 29, 2017
Merged

fix: Menu jumps vertically on logout #3194

merged 4 commits into from Sep 29, 2017

Conversation

tsl143
Copy link
Contributor

@tsl143 tsl143 commented Sep 21, 2017

Fixes: mozilla/addons#10792

Fixed GIF
menu

@tsl143
Copy link
Contributor Author

tsl143 commented Sep 21, 2017

@muffinresearch, yarn nsp-check even fails at master branch. Did we merge something break-ish ?

@tofumatt
Copy link
Contributor

tofumatt commented Sep 21, 2017

This seems fine, I'm just confirming it doesn't change anything on other screen sizes...

Copy link
Contributor

@tofumatt tofumatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still moves around on mobile:

2017-09-21 11 41 24

Could you adjust it so it doesn't jump on smaller displays as well?

@tofumatt
Copy link
Contributor

(that's responsive mode in Firefox at 320 x 480, btw)

@codecov-io
Copy link

codecov-io commented Sep 21, 2017

Codecov Report

Merging #3194 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/core/api/search.js 100% <0%> (ø) ⬆️
src/core/searchUtils.js 100% <0%> (ø) ⬆️
src/core/api/index.js 96.82% <0%> (+0.05%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4b89e6...80df25d. Read the comment docs.

@tsl143
Copy link
Contributor Author

tsl143 commented Sep 21, 2017

@tofumatt , I got confused between screen size variables then I got mobile-first concept :)
That's cool, and I am going to use that in my projects too.

@muffinresearch
Copy link
Contributor

muffinresearch commented Sep 21, 2017

@muffinresearch, yarn nsp-check even fails at master branch. Did we merge something break-ish ?

See mozilla/addons#10799

padding-bottom: 6px;
padding-top: 8px;
padding-bottom: 2px;
padding-top: 4px;
Copy link
Member

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.

Copy link
Member

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;
Copy link
Member

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?

Copy link
Contributor Author

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:
mobile

Desktop:
ezgif-4-2d44bd1686

Copy link
Member

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 :-(

@willdurand
Copy link
Member

Fixing the height on mobile devices make it work like a charm. However, this broke the desktop mode (focus lost when trying to reach the menu from the username):

2017-09-26 13 00 14

@willdurand
Copy link
Member

@tsl143 sorry, can you try adding height: 30px for desktop? (in @include respond-to(large))

@tsl143
Copy link
Contributor Author

tsl143 commented Sep 26, 2017

@willdurand Instead, how about adding height: auto there?

@willdurand
Copy link
Member

@willdurand Instead, how about adding height: auto there?

if that works, why not. I tested locally with 30px to be honest, so I now it should be fine. I'll test with auto then :)

Copy link
Member

@willdurand willdurand left a 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 👍

@kumar303
Copy link
Contributor

@willdurand has confirmed that the issue in #3194 (review) is resolved.

Thanks @tsl143 !

@kumar303 kumar303 merged commit 3f3e0d2 into mozilla:master Sep 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants