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: Fix inserter misalignment regression #10461

Merged
merged 3 commits into from Oct 10, 2018

Conversation

Projects
None yet
4 participants
@tofumatt
Member

tofumatt commented Oct 10, 2018

Fix #10453

Description

Fixed the visual regression caused in #10295 by moving the margin fix to target the NUX tooltips specifically.

How has this been tested?

Tested in RTL and LTR, in Firefox, mobile and larger viewports.

Screenshots

Before

screenshot 2018-10-09 20 52 41

After

screenshot 2018-10-10 01 30 52

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

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Oct 10, 2018

@talldan

Looks good! The only thing I spotted is that prior to #10295 the margin for the popver used to be -24px, and we might want to restore it to it's original value:
https://github.com/WordPress/gutenberg/pull/10295/files#diff-dcb8eefee4aaf82c9ddb042494547527L174

@jasmussen

This needs slightly more time in the oven. Just noting here so you don't merge, will follow up with a comment shortly.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 10, 2018

Member

Looks good! The only thing I spotted is that prior to #10295 the margin for the popver used to be -24px, and we might want to restore it to it's original value:
https://github.com/WordPress/gutenberg/pull/10295/files#diff-dcb8eefee4aaf82c9ddb042494547527L174

I saw that but didn't notice a difference when setting it… but maybe I set the wrong one and in general I found that CSS to be a bit hard to keep track of. 🤷‍♂️ I'll try again, after @jasmussen tells me what I undercooked! 😉

Member

tofumatt commented Oct 10, 2018

Looks good! The only thing I spotted is that prior to #10295 the margin for the popver used to be -24px, and we might want to restore it to it's original value:
https://github.com/WordPress/gutenberg/pull/10295/files#diff-dcb8eefee4aaf82c9ddb042494547527L174

I saw that but didn't notice a difference when setting it… but maybe I set the wrong one and in general I found that CSS to be a bit hard to keep track of. 🤷‍♂️ I'll try again, after @jasmussen tells me what I undercooked! 😉

@tofumatt tofumatt self-assigned this Oct 10, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 10, 2018

Contributor

Thanks so much for working on this, nice work.

I'm happy to see that this also appears to fix a regression with tooltips that's currently present in master:

screenshot 2018-10-10 at 09 55 46

This branch:

screenshot 2018-10-10 at 10 06 37

3.9:

screenshot 2018-10-10 at 10 07 38

But sadly there is a little more work to be done.

This branch:

screenshot 2018-10-10 at 10 08 32

3.9:

screenshot 2018-10-10 at 10 04 40

Notice how in this branch, the triangle is way on the edge, whereas in 3.9 it's indented a bit. To be perfectly precise the triangle is right, but the popout is not.

This also manifests in other usage of the popout component. This branch:

screenshot 2018-10-10 at 10 04 54

3.9:

screenshot 2018-10-10 at 10 04 48

This branch:

screenshot 2018-10-10 at 10 05 02

3.9:

screenshot 2018-10-10 at 10 05 08

HOPEFULLY the fix is as simple as using a negative margin to position the content, rather than a zero margin. But let me know if you need any help and I can jump in.

Contributor

jasmussen commented Oct 10, 2018

Thanks so much for working on this, nice work.

I'm happy to see that this also appears to fix a regression with tooltips that's currently present in master:

screenshot 2018-10-10 at 09 55 46

This branch:

screenshot 2018-10-10 at 10 06 37

3.9:

screenshot 2018-10-10 at 10 07 38

But sadly there is a little more work to be done.

This branch:

screenshot 2018-10-10 at 10 08 32

3.9:

screenshot 2018-10-10 at 10 04 40

Notice how in this branch, the triangle is way on the edge, whereas in 3.9 it's indented a bit. To be perfectly precise the triangle is right, but the popout is not.

This also manifests in other usage of the popout component. This branch:

screenshot 2018-10-10 at 10 04 54

3.9:

screenshot 2018-10-10 at 10 04 48

This branch:

screenshot 2018-10-10 at 10 05 02

3.9:

screenshot 2018-10-10 at 10 05 08

HOPEFULLY the fix is as simple as using a negative margin to position the content, rather than a zero margin. But let me know if you need any help and I can jump in.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 10, 2018

Member

❤️ for all the info. I'll get on this after espresso.

Member

tofumatt commented Oct 10, 2018

❤️ for all the info. I'll get on this after espresso.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 10, 2018

Contributor

No worries, and like I said if the CSS gives you headaches, reach out and we'll work on it together.

Contributor

jasmussen commented Oct 10, 2018

No worries, and like I said if the CSS gives you headaches, reach out and we'll work on it together.

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Oct 10, 2018

Contributor

@tofumatt Setting the margin to -24px should fix the issue mentioned by @jasmussen 😄

Perhaps should have been clearer in my message about what the differences were.

Contributor

talldan commented Oct 10, 2018

@tofumatt Setting the margin to -24px should fix the issue mentioned by @jasmussen 😄

Perhaps should have been clearer in my message about what the differences were.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 10, 2018

Member

Unfortunately not, it introduces a new issue:

2018-10-10 11 28 12

The popover on the edge of the screen gets pushed out-of-bounds so there's no border.

Using $large-grid-size looks okay by me and stays on the screen. It's 16px. How about that?

2018-10-10 11 29 17

Member

tofumatt commented Oct 10, 2018

Unfortunately not, it introduces a new issue:

2018-10-10 11 28 12

The popover on the edge of the screen gets pushed out-of-bounds so there's no border.

Using $large-grid-size looks okay by me and stays on the screen. It's 16px. How about that?

2018-10-10 11 29 17

@tofumatt tofumatt requested a review from jasmussen Oct 10, 2018

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Oct 10, 2018

Contributor

Good catch! That looks good to me. @jasmussen?

Also, since #10430 was merged earlier, it might be worth rebasing if you haven't already. I don't know if it'll effect this PR, but it does change the padding in those menus.

Contributor

talldan commented Oct 10, 2018

Good catch! That looks good to me. @jasmussen?

Also, since #10430 was merged earlier, it might be worth rebasing if you haven't already. I don't know if it'll effect this PR, but it does change the padding in those menus.

tofumatt added some commits Oct 10, 2018

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 10, 2018

Contributor

Looking now.

Contributor

jasmussen commented Oct 10, 2018

Looking now.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 10, 2018

Member

Rebased and did not notice any issues.

On very small screens the issue above (the "…" menu's edge being pushed off-screen) still happens, but that seems to be an existing thing and this is at least better for most screen sizes.

Member

tofumatt commented Oct 10, 2018

Rebased and did not notice any issues.

On very small screens the issue above (the "…" menu's edge being pushed off-screen) still happens, but that seems to be an existing thing and this is at least better for most screen sizes.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 10, 2018

Contributor

I have to admit, I vastly prefer the negative margin of 24px to $grid-size-large, even if it means the ellipsis menu is cut off on the right. Here's 24px:

screenshot 2018-10-10 at 12 55 43

screenshot 2018-10-10 at 12 55 50

screenshot 2018-10-10 at 12 55 53

screenshot 2018-10-10 at 12 56 04

In this, everything seems more balanced and attached.

While it would be nice for everything to be on a base8 grid, given that we're not at the time, perhaps we can compromise and land on 20px? Here's 20:

screenshot 2018-10-10 at 12 59 40

(flush against the edge there!)

screenshot 2018-10-10 at 12 59 45

screenshot 2018-10-10 at 12 59 50

screenshot 2018-10-10 at 12 59 54

Contributor

jasmussen commented Oct 10, 2018

I have to admit, I vastly prefer the negative margin of 24px to $grid-size-large, even if it means the ellipsis menu is cut off on the right. Here's 24px:

screenshot 2018-10-10 at 12 55 43

screenshot 2018-10-10 at 12 55 50

screenshot 2018-10-10 at 12 55 53

screenshot 2018-10-10 at 12 56 04

In this, everything seems more balanced and attached.

While it would be nice for everything to be on a base8 grid, given that we're not at the time, perhaps we can compromise and land on 20px? Here's 20:

screenshot 2018-10-10 at 12 59 40

(flush against the edge there!)

screenshot 2018-10-10 at 12 59 45

screenshot 2018-10-10 at 12 59 50

screenshot 2018-10-10 at 12 59 54

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 10, 2018

Member
Member

tofumatt commented Oct 10, 2018

@jasmussen

With 24px it looks to me the same as it did before the regression. This is a good baseline to get back to. I'm sure separate iterations will look at popovers.

@tofumatt tofumatt merged commit 49ed442 into master Oct 10, 2018

2 checks passed

codecov/project 49.48% remains the same compared to 085a605
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the fix/10453-inserter-alignment branch Oct 10, 2018

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