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 UnderlineNav overflow #684

Merged
merged 4 commits into from
Feb 20, 2019
Merged

Fix UnderlineNav overflow #684

merged 4 commits into from
Feb 20, 2019

Conversation

shawnbot
Copy link
Contributor

@shawnbot shawnbot commented Feb 20, 2019

This makes some tweaks to the UnderlineNav component:

  1. Adds overflow-x: auto for horizontal scrolling, and overflow-y: hidden to prevent vertical scrolling.
  2. Removes 1px clipping via margin-bottom: -1px on .UnderlineNav-body (because: why?)

I'm requesting reviews from folks who have lots of experience with this component. Thanks!

@shawnbot shawnbot mentioned this pull request Feb 20, 2019
10 tasks
@shawnbot
Copy link
Contributor Author

shawnbot commented Feb 20, 2019

This appears to be working:

desktop mobile
image image

@ampinsk
Copy link
Contributor

ampinsk commented Feb 20, 2019

Just for context, the margin-bottom: -1px on .UnderlineNav-body was so that the orange line sat on top of the gray line, instead of right next to each other:

screen shot 2019-02-20 at 1 14 11 pm

screen shot 2019-02-20 at 1 14 47 pm

If it's causing problems then I don't think it's too much trouble to get rid of it! But is nice in theory 😄

@shawnbot
Copy link
Contributor Author

@ampinsk thanks, that's helpful! I'm going to revert that commit.

Copy link
Contributor

@emilybrick emilybrick left a comment

Choose a reason for hiding this comment

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

This lgtm! Once this ships I will remove the custom css I shamefully added here 😬

@shawnbot shawnbot merged commit 1f85792 into release-12.0.1 Feb 20, 2019
@shawnbot shawnbot deleted the underline-nav-overflow branch February 20, 2019 23:28
@shawnbot
Copy link
Contributor Author

Welp, this turns out not to have been a good change after all. overflow-y: hidden actually hides the lower half of the underline:

image

I tried a couple of things in the inspector and failed. I think getting the underline to overlap the border is going to require some more work, so in the meantime @ampinsk, are you okay with nixing the margin-bottom: -1px? 😬

@ampinsk
Copy link
Contributor

ampinsk commented Feb 21, 2019

Yeah that's fine! It's a small detail so if it's a big headache we should nix it 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants