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 guided tour popup margins #10295

Merged
merged 4 commits into from Oct 8, 2018

Conversation

Projects
None yet
2 participants
@nielslange
Contributor

nielslange commented Oct 2, 2018

Description

Removed negative left and right margins from guided tour popup when scrolling on mobile devices.

How has this been tested?

Fix had been tested using iPhone 7 Plus with iOS 11.4.1 in both English (LTR) and Hebrew (RTL).

Screenshots

Before

After

Types of changes

Removed two negative margins (LTR & RTL) within gutenberg/packages/components/src/popover/style.scss

Checklist:

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

@melchoyce melchoyce requested a review from karmatosed Oct 2, 2018

@tofumatt

Thanks for the patch!

I think this makes sense and looks good, but I'm a bit confused about the screenshots you've submitted. It looks like there are "before" and "after" screenshots, but I can't tell which ones are which so it's hard to tell what the change is here.

Could you edit your PR comment to clarify the before/after screenshots and have a look at the comments I made? After that this should be good to merge 😄

Show outdated Hide outdated package-lock.json Outdated
}
.components-popover.edit-post-more-menu__content:not(.is-mobile):not(.is-middle).is-right & {
/*!rtl:ignore*/

This comment has been minimized.

@tofumatt

tofumatt Oct 2, 2018

Member

Could you add some comments explaining why these should be ignored? Maybe even just a link to this PR, just so future developers will understand.

@tofumatt

tofumatt Oct 2, 2018

Member

Could you add some comments explaining why these should be ignored? Maybe even just a link to this PR, just so future developers will understand.

This comment has been minimized.

@nielslange

nielslange Oct 3, 2018

Contributor

*!rtl:ignore*/ came from the CSS statement before, which I copied and adjusted.

@nielslange

nielslange Oct 3, 2018

Contributor

*!rtl:ignore*/ came from the CSS statement before, which I copied and adjusted.

@tofumatt tofumatt removed the request for review from karmatosed Oct 3, 2018

@nielslange

This comment has been minimized.

Show comment
Hide comment
@nielslange

nielslange Oct 3, 2018

Contributor

@tofumatt Thanks for your feedback. I made the screenshots smaller and added title tags to it, thus, when you hover over the images, you'll see their titles and will reply to your other notes directly in the related sections.

Contributor

nielslange commented Oct 3, 2018

@tofumatt Thanks for your feedback. I made the screenshots smaller and added title tags to it, thus, when you hover over the images, you'll see their titles and will reply to your other notes directly in the related sections.

@nielslange

This comment has been minimized.

Show comment
Hide comment
@nielslange

nielslange Oct 8, 2018

Contributor

@tofumatt Besides the screenshots you've requested, is there anything else I should change in order to get this PR merged? 😉

Contributor

nielslange commented Oct 8, 2018

@tofumatt Besides the screenshots you've requested, is there anything else I should change in order to get this PR merged? 😉

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 8, 2018

Member

Nope, sorry, just didn't see this was ready for another review; looking now!

Member

tofumatt commented Oct 8, 2018

Nope, sorry, just didn't see this was ready for another review; looking now!

@tofumatt

Those are some epic selectors, but it was like that already, so 🤷‍♂️

Thanks for the patch! Once Travis passes I'll get things merged.

@tofumatt tofumatt added this to the 4.0 milestone Oct 8, 2018

@tofumatt tofumatt merged commit f1d4fcb into WordPress:master Oct 8, 2018

2 checks passed

codecov/project 49.31% (-0.13%) compared to 020b360
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nielslange

This comment has been minimized.

Show comment
Hide comment
@nielslange

nielslange Oct 9, 2018

Contributor

Great to see this PR got merged, @tofumatt! It's my first contribution to Gutenberg and most likely not the last. 😀

Contributor

nielslange commented Oct 9, 2018

Great to see this PR got merged, @tofumatt! It's my first contribution to Gutenberg and most likely not the last. 😀

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