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

Allow margins to collapse & refactor block toolbar #7365

Merged
merged 9 commits into from Jul 5, 2018

Conversation

@jasmussen
Contributor

jasmussen commented Jun 19, 2018

The primary purpose of this PR is to allow margins to collapse.

When two margins meet, and no borders or paddings are present, they collapse. I.e. a p with a 20px margin next to a similar p will have only 20px margin between them, not 40px, because the two siblings collapse to whichever margin is the largest.

By allowing margins on blocks to collapse, we can make it easier to theme the editor to look like the frontend, by making margins behave the same.

This PR also refactors how the block toolbar is positioned. Because we use position: sticky;, the toolbar is part of the flow of the block itself. In this PR it uses translate to pull it out of the flow, meaning the block metrics do not have to account for the presence of the block toolbar or not. This is a nice refactor on its own, but it also makes the positioning more resilient and predictable.

Testing

Because this is a rather large change to a visible part of the editor, it needs a good amount of testing. The gold standard is that the editor in this branch should look no different than the editor in master.

So please test on all breakpoints, test the frontend and backend, test with fixed toolbar and contextual toolbar:

  • Test that the block toolbar that looks right on all blocks, including blocks like the spacer that do not have a block toolbar.
  • Test all blocks they look right, margin-wise.
  • Test in nested contexts blocks, test child blocks.
  • Test with editor styles.

It's okay that there are a few regressions with existing editor styles, the benefits outweigh any fixes that need to be made.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 21, 2018

This has been rebased, and I feel like it's in solid shape for wider testing, and I think we should get it in 3.2. Aside from the code cleanliness and benefits this has for editor-themers, the way the block outlines and toolbar are painted now are more resilient. Because it's hard to explain, here are two GIFs.

Here, I am adjusting the margins on the block content itself, in master, where margins do not collapse:

without

Note how the block toolbar does not correctly position itself according to the adjusted margins.

Here's from the branch that allows collapsing margins:

with

Note how changing just one margin on the block content doesn't do anything — because they collapse so the remaining margin is still there. Also notice how when the other margin is changed, the outlines and block toolbar correctly position themselves.

@aduth

Block toolbar is very misplaced in IE11

image

@@ -210,7 +209,8 @@
/**
* Block outline layout
*/
.editor-block-list__block-edit {
.editor-block-list__block-edit {

This comment has been minimized.

@aduth

aduth Jun 25, 2018

Member

Mixed whitespace. Preceding space should be removed.

This comment has been minimized.

@jasmussen

jasmussen Jun 26, 2018

Contributor

Nice catch, thank you.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 26, 2018

Addressed feedback. Fixed the whitespace (nice catch, it shows up as a dot in VS code, I should've seen it but somehow missed it), and made it work in IE again. I hadn't properly rebased for Brandon's fix earlier, but now it works:

edge

ie

Noting also for others as well as myself, that @supports( position: sticky ) { ... } seems like a great way to target IE11 but not Edge, even for issues outside of position sticky. For example I might want to refactor the gallery hack that I made, to make things work in IE11, to use the above instead.

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Jun 27, 2018

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 28, 2018

Rebased, and squashed to keep future rebases simpler. In a fixup'ed commit, https://github.com/WordPress/gutenberg/pull/7365/files#diff-00ebf096f7c483426f2a78d70d9cbd22R1003, I included a fix for a horizontal scrollbar regression introduced in #7225.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jun 28, 2018

Unsure why Travis fails here. The tests pass.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 28, 2018

That has been intermittently failing a lot. I've restarted it. 😢

@karmatosed

Design side approving and did some testing. I would like to have more testing just to be super sure.

@jorgefilipecosta

This comment has been minimized.

Member

jorgefilipecosta commented Jul 3, 2018

Hi @jasmussen, thank you for this changes. I confirm it solves a regression introduced during the refactoring to the block hover area.
I noticed a regression when clicking on the toolbar even if we move the mouse one pixel we trigger multi-selection. Before that was not the case. I'm not certain what is causing this problem maybe solve z-index change.
jul-03-2018 16-09-54

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 3, 2018

FYI fixed the merge conflict here (via git rebase, so anyone working on this branch'll need to pull this new version).

@tofumatt

This works for me from my testing, but I noticed the same regression @jorgefilipecosta did.

I found the cause… I'll push a commit fix for it.

@@ -522,7 +523,7 @@ export class BlockListBlock extends Component {
className="editor-block-list__block-edit"
data-block={ uid }
>
{ shouldShowContextualToolbar && <BlockContextualToolbar /> }

This comment has been minimized.

@tofumatt

tofumatt Jul 4, 2018

Member

Did a quick test of this locally, and the relocation of this component seems to be what's causing the selection issue @jorgefilipecosta pointed out. Moving it back fixes the issue and doesn't seem to cause any display bug issues.

This comment has been minimized.

@jasmussen

jasmussen Jul 4, 2018

Contributor

This move was initially the key to unlocking collapsing margins. So I assumed that aspect would break when you moved this back.

However later on I added 3c0afac#diff-00ebf096f7c483426f2a78d70d9cbd22R904, which as it turns out is sufficient to enable collapsing margins, even without being relocated.

However all the in-between styles were written to assume the position of the nested toolbar where it was before this move, so there were a ton of visual regressions as a result of this move, like toolbars not being in the right place, and so on. But I'm fixing those now, and in the end I think it was good to move this back. Thanks for all the help.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 4, 2018

I pushed a fix (cb8aae8) for the regression with mouse movement on mouseDown @jorgefilipecosta mentioned and rebased this against master as there have been a few conflicts.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jul 4, 2018

Okay, so I fixed a ton of issues that propped up after the columns merge and the fix to the mouse movement regression. So many that I think this needs another deep dive into the code. Can you take a look, @tofumatt? Thanks.

I left comments for each fix in each commit.

Once again tests pass for me with npm run test but Travis fails. Not sure why.

Also, in master and this branch, every image is broken for me, even after an npm install, I'm getting this error:

react-dom.min.82e21c65.js:110 TypeError: this.props.forwardedRef is not a function
    at Wrapper.handleRef (index.js:58)
    at cg (react-dom.min.82e21c65.js:110)
    at dg (react-dom.min.82e21c65.js:110)
    at eg (react-dom.min.82e21c65.js:113)
    at jc (react-dom.min.82e21c65.js:131)
    at gc (react-dom.min.82e21c65.js:127)
    at vb (react-dom.min.82e21c65.js:126)
    at ub (react-dom.min.82e21c65.js:126)
    at zd (react-dom.min.82e21c65.js:124)
    at ra (react-dom.min.82e21c65.js:123)

But given that's also in master, I'm guessing it's unrelated to this branch.

@tofumatt tofumatt self-requested a review Jul 4, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 4, 2018

npm install works for me so I'm guessing it's local to your install, and that failing travis test is… frequent and happening a lot. We'll need to look into it. 😢

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jul 4, 2018

To clarify, npm install is working fine for me. What isn't, is any image or embed. Try loading the demo content, I'm pretty sure you should see the same issue.

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 4, 2018

Shoot, sorry, I misread. You're right: I see that too...

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 4, 2018

As mentioned in Slack, this seems unrelated but a legit breakage: https://wordpress.slack.com/archives/C02QB2JS7/p1530703711000116

@tofumatt

tofumatt approved these changes Jul 4, 2018 edited

I like this a lot more, it feels more like a real, empty canvas (and more like the old editor) with the tighter margins. I have no idea what I'm on about and I think that's an unrelated change. I see what this is doing now. All good. More coffee needed.

I think we need to sort out that image bug but as mentioned it's not related to this PR and seems to be happening in master.

🚢 … but maybe after we fix master just so we don't merge breaking builds 😉

.editor-default-block-appender__content,
.editor-block-list__block .editor-block-list__block-edit,
.editor-block-list__layout .editor-block-list__block .editor-block-list__block-edit,
.editor-block-list__layout .editor-default-block-appender .editor-default-block-appender__content { // Explicitly target nested blocks, as those need to override the preceeding rule.

This comment has been minimized.

@tofumatt

tofumatt Jul 4, 2018

Member

Typo: "preceding". English is weird. I've pushed a fix 😄

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jul 4, 2018

As mentioned in Slack though: this is a great example of a PR that should include before/after screenshots. The differences are easy to spot visually but I found the lack of issue to make it unclear what I was testing for.

I need more coffee. Ignore me.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jul 4, 2018

Thanks a bunch for the review. I'll wait for master to behave and then merge, possibly rebase first.

jasmussen and others added some commits Jun 12, 2018

Allow collapsing margins & adjust block toolbar
This PR refactors how the block toolbar is positioned. Because we use sticky, the toolbar is part of the flow of the block itself. In this PR it uses translate to pull it out of the flow, making the positioning more resilient.

The primary purpose of this PR is to allow margins to collapse. When two margins meet, and no borders or paddings are present, they collapse. I.e. a `p` with a 20px margin next to a similar `p` will have only 20px margin between them, not 40px, because the two siblings collapse to whichever margin is the largest. By allowing margins on blocks to collapse, we can make it easier to theme the editor to look like the frontend.
Fix rebase issues, part 1
- Fixed toolbar position on mobile.
- Fixed toolbar position on desktop breakpoints
- Fixed appender regression
- Fixed issue with warning blocks having no hover or selected outlines.
- Fixed positioning of ellipsis on wide blocks.
- Fixed issue with full-wide gallery.
Fix rebase issues, part 2.
- Fixed a few typos and an issue with warnings. This will be revisited in a future patch also being worked on separately.
- Fixed position of toolbar on wide and fullwide.
- Fixed positioning of mover on wide and fullwide.
- Fixed issue with additional margins in columns block.
- Fixed issue with the invisible toolbar on the Column (singular) block breaking the layout. This also fixed an issue with the hover label and sibling inserter positioning on the column.
Fix rebase issues, part 3.
- Fixed a regression in IE.
- Fixed the position of editor warning after the refactor.
@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Jul 4, 2018

Rebased. Images work now, and the rest does too.

How do we figure out if the travis failure is real? And otherwise, can we merge?

@gziolo

This comment has been minimized.

Member

gziolo commented Jul 5, 2018

How do we figure out if the travis failure is real? And otherwise, can we merge?

It would be very surprising if CSS changes would make any test fail. I retriggered another build.

position: sticky;
z-index: z-index( '.editor-block-contextual-toolbar' );
white-space: nowrap;
text-align: left;
pointer-events: none;
height: $block-toolbar-height;
//height: $block-toolbar-height;

This comment has been minimized.

@gziolo

gziolo Jul 5, 2018

Member

We should remove this line before merging.

This comment has been minimized.

@jasmussen

jasmussen Jul 5, 2018

Contributor

Fixed, thanks. I suppose this triggered another test? :)

jasmussen and others added some commits Jul 5, 2018

@gziolo gziolo merged commit 06a621d into master Jul 5, 2018

2 checks passed

codecov/project 46.42% (-0.03%) compared to 6bf9832
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the try/allow-collapsing-margins-v3 branch Jul 5, 2018

it( 'Should insert content using the placeholder and the regular inserter', async () => {
// Click below editor to focus last field (block appender)
await clickBelow( await page.$( '.editor-default-block-appender' ) );
await page.click( '.editor-writing-flow__click-redirect' );

This comment has been minimized.

@aduth

aduth Oct 29, 2018

Member

This refactor may have made the test less effective at covering the expected behavior: #11168 (review)

This comment has been minimized.

@jasmussen

jasmussen Oct 30, 2018

Contributor

What's the best way to enhance this test again? CC: @gziolo who graciously helped me with this.

This comment has been minimized.

@gziolo

gziolo Oct 30, 2018

Member

Ooops, I wasn't aware of that :(

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