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 modal header overlapping the border #10442

Closed
wants to merge 3 commits into from

Conversation

johngodley
Copy link
Contributor

A change in #9900 has made the heading in a modal overlap with the top border:

edit_post_ wordpress_latest _wordpress

This is only visible on a small screen.

This PR tweaks the top offset, pushing it down by the border width:

edit_post_ wordpress_latest _wordpress

It fixes the small screen problem and doesn't appear to negatively affect a desktop screen (although this could be tweaked further)

How has this been tested?

Set display to a small screen and open the keyboard shortcuts modal. Verify the heading overlaps and that the PR fixes the overlap.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

On mobile the modal’s header was overlapping the modal border
@johngodley johngodley added the [Type] Regression Related to a regression in the latest release label Oct 9, 2018
@johngodley johngodley requested a review from jasmussen October 9, 2018 15:02
@jasmussen
Copy link
Contributor

This is actually a rounding error by the browser, which positions vertically using translate and therefore can place the modal on subpixels if the viewport width is an uneven number.

To put it differently, the position is actually correct in master, but due to the subpixel blurring, there's that little bit of overlap. When you push it down like in this branch, you can see the text underneath as you scroll:

screenshot 2018-10-10 at 08 52 11

I pushed a fix that I believe improves this. Expanding review range for a sanity check for both of us.

@johngodley
Copy link
Contributor Author

I pushed a fix that I believe improves this

👍 Looks good in my testing

@johngodley
Copy link
Contributor Author

I can no longer reproduce the original problem so this seems to have got fixed. Closing

@johngodley johngodley closed this Nov 12, 2018
@johngodley johngodley deleted the fix/modal-header-border branch November 12, 2018 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants