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 newly discovered CSS issues #812

Merged
merged 1 commit into from
Jun 30, 2018
Merged

Fix newly discovered CSS issues #812

merged 1 commit into from
Jun 30, 2018

Conversation

yangshun
Copy link
Contributor

Motivation

Address the CSS issues uncovered by @chenglou in reasonml/reasonml.github.io#398.

Have you read the Contributing Guidelines on pull requests?

Yes

Test Plan

  • Check that search input looks ok on all resolutions.
  • Edit button vertically-aligned with the heading.
  • Edit button hidden <1024px.

Related PRs

reasonml/reasonml.github.io#398

@yangshun yangshun requested review from chenglou and endiliey June 29, 2018 08:19
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jun 29, 2018
@yangshun
Copy link
Contributor Author

yangshun commented Jun 29, 2018

@JoelMarcey We'll release v1.3.1 after this PR is merged.

@docusaurus-bot
Copy link
Contributor

Deploy preview for docusaurus-preview ready!

Built with commit a334a4b

https://deploy-preview-812--docusaurus-preview.netlify.com

Copy link
Contributor

@endiliey endiliey left a comment

Choose a reason for hiding this comment

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

LGTM. Just realized we use 1023px and sometime 1024px in our css.

I am also not sure why we choose 735px for some part in our css, I thought the standard medium devices is 768px.

@yangshun
Copy link
Contributor Author

Just realized we use 1023px and sometime 1024px in our css.

They should be max-width: 1023px and min-width: 1024px as the breakpoint. If you see something else, then it's 1px off.

I am also not sure why we choose 735px for some part in our css, I thought the standard medium devices is 768px.

Hmm I think it's iPhone 6, 7, 8+ landscape mode width. Source: https://css-tricks.com/snippets/css/media-queries-for-standard-devices/

Anyway in future we can remove 736 and just anything below 1024 we show mobile view.

@yangshun yangshun merged commit 0b10b19 into facebook:master Jun 30, 2018
@yangshun yangshun deleted the css branch June 30, 2018 07:27
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.

4 participants