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 search bar width for mobile nav #575

Merged
merged 3 commits into from
Apr 17, 2018
Merged

Fix search bar width for mobile nav #575

merged 3 commits into from
Apr 17, 2018

Conversation

yangshun
Copy link
Contributor

@yangshun yangshun commented Apr 17, 2018

Motivation

Fixes #571. Shorten search bar by 10% on mobile since it doesn't really make too big of a visual difference being shorter.

This is just a quick fix and the nav bar CSS could probably be improved. There's no need for a min-device-width in the media query as the search bar should probably be shortenened for devices below the min-device-width as well.

This fix shouldn't affect existing users as shortening the search bar should be a backward-compatible change.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

Tested on latest device resolutions in Chrome Devtools. This width works well on all of the devices available, even iPhone 5S, which is only 320px wide.

The following screenshots were taken on Pixel 2.

Before

After

@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Apr 17, 2018
Copy link
Contributor

@JoelMarcey JoelMarcey left a comment

Choose a reason for hiding this comment

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

I ❤️it when someone who knows design a lot better than me fixes this stuff.

Thanks!

@JoelMarcey JoelMarcey merged commit ba024a2 into facebook:master Apr 17, 2018
@SimenB
Copy link
Contributor

SimenB commented Apr 17, 2018

Will this work if the project is named "this is a stupidly long name 🤷‍♂️"?

EDIT:

This is just a quick fix and the nav bar CSS could probably be improved.

Oh

@JoelMarcey
Copy link
Contributor

@SimenB Yeah, it might not fix in all cases, possibly. But it should be a good temp fix for short/normally name projects

I think what we probably want to do is put the versioning with the header links or something. Or maybe even remove the project name from the top row. Something like that.

@JoelMarcey
Copy link
Contributor

In other words - we are at a better state than we were with this fix; not perfect yet. Worth merging though.

@SimenB
Copy link
Contributor

SimenB commented Apr 17, 2018

Yeah, just removing the project name altogether on lower resolutions sounds good

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

Successfully merging this pull request may close these issues.

Version in header clashes with search box on mobile
4 participants