-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Try: Refactor block margins #5217
Conversation
@@ -628,6 +606,28 @@ export class BlockListBlock extends Component { | |||
aria-label={ blockLabel } | |||
data-block={ block.uid } | |||
> | |||
<BlockDropZone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is not pretty, and I will need some help making sure this works as well as it needs to. But the reason for moving all of this is that it's necessary in order to allow the margins to collapse. Quite simply, all these elements have to be children of the content element itself, otherwise the content elements margins will be uncollapsed by the presence of adjecent divs. See also this codepen: https://codepen.io/joen/pen/aqKLqa?editors=1100
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know exactly why these components are outside of this div but what I know is that this block's content wrapper is targeted in code in a lot of places, so this change could have side effects. I'm seeing issues with the movers and with multi-selection in this branch.
Do you think it's possible to use another trick to move the margins to the parent wrapper instead, I'd feel more confident with the changes, if we can do so instead?
This doesn't mean we can't move these here, just that there are probably consequences I'm not aware of.
--
Also, do you think it's possible to split out some of the enhancements of this PR to separate smaller PRs, this would ease reviewing testing and mergin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the thoughts.
I think you've confirmed for me that it's a good idea to split this up into the general enhancements and refactor, and the collapsing margins separately, to let us focus. I will do that.
Do you think it's possible to use another trick to move the margins to the parent wrapper instead, I'd feel more confident with the changes, if we can do so instead?
I don't know, but I don't think so. In https://codepen.io/joen/pen/aqKLqa I basically tried finding the minimal layout we could use, to keep the block-docked toolbar and allow the margins to collapse. Basically, as soon as the block whose margins need to be allowed to collapse gets an adjecent element, like the block toolbar, those margins uncollapse.
* Selected Block style | ||
*/ | ||
|
||
.editor-block-list__block-edit>div:not( .components-drop-zone ):not( .editor-block-mover ):not( .editor-block-settings-menu ):not( .editor-block-contextual-toolbar ):not( .editor-block-list__block-mobile-toolbar ) { // @todo — target a classname, not this crazy "not" rule |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After moving all the Block UI elements around, there is no longer just a single div inside .editor-block-list__block-edit
, there are multiple. And the one we need to paint a border around has no CSS class. If we decide that the approach taken in this PR is worth pursuing, then we need to add a CSS class, so we don't have to use this insane CSS :not
rule. Something like editor-block-list__block-edit__content
, perhaps?
z-index: z-index( '.editor-block-list__block:before' ); | ||
content: ''; | ||
position: absolute; | ||
top: -$block-padding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's quite nice to be able to use negative values here. That means we paint the outline around the content itself, allowing the content margins to position and space items. The border is just painted around that, as is also shown in https://codepen.io/joen/pen/aqKLqa.
This should benefit nested UI also, as it will be one less bit of UI to "offset" in nested contexts.
Pushed a fix to floats right at the start. Aside from a number of issues, on a high level, this feels like it's working reasonably well. Some of those issues include:
However before I spend too much time working on this, I would really appreciate some eyes on this, so we can decide whether it's worth it, and whether there are other endemic issues to this approach that should be considered. If we do decide it's worth it, then I would appreciate help with #5217 (comment), as that's a blocker for fixing some of the selection issues. |
Pushed another fix. Now there is only a single max-width on the main column. This allows us to eventually utilize the In addition to this, it uses negative margins on the side UI to pull it out. It still works on hover, but the chief benefit to this is that blocks have a true width that isn't affected by side UI. That means if your column is 100px wide, your block inside it is 100px wide. This aspect is also not perfect, but it's yet another stepping stone: CC: @mtias, would appreciate your eyes on this. Going to step away from it until we can decide. |
620996b
to
06b25d7
Compare
Rebased and tweaked the in-between inserter as well. This partially duplicating work from #5282. CC @youknowriad |
f1a82ac
to
7784a1a
Compare
There is a lot of good stuff in this branch that isn't directly related to the collapsing margins. Things like:
However there are also some fairly big regressions in this branch, very probably as a result of me moving around a bunch of the block helper UI here: https://github.com/WordPress/gutenberg/pull/5217/files#r170248974 Things like the mobile toolbar not showing up on mobile (somehow There are also cosmetic issues/smaller things, like:
But the bottomline is, I'm wondering whether I should split this PR in two. One that includes the mostly CSS structural stuff and improvements to width specific aspects. Then I can try and revisit the height related collapsing margins stuff separately. It is a bit difficult to untangle, but it may be necessary to do an in-between step, so we can sort of compartmentalize the collapsing margin work (which requires the rearranging of toolbars/items from https://github.com/WordPress/gutenberg/pull/5217/files#r170248974). |
CC: @youknowriad if you have time to look at the code, notably https://github.com/WordPress/gutenberg/pull/5217/files#r170248974, I would appreciate your opinion:
This will help me decide whether I should revert the collapsing-margin specific aspects of this PR, get it in, and then try and tackle that again separately. Thanks. |
This PR is now ready for a good code-review. The short of it is this: I believe it is solid, and brings lots of improvements to how blocks are rendered in our editor. We should not put it in 2.3, but it would be nice to put in 2.4. Changes in this PR: Blocks now retain their standard dimensions. The side UI and the fact that these have to work with hover, does not affect block dimensions anymore, through a combination of paddings and negative margins. This has allowed us to change the main column width to be just a single number, now called Then negative margin trick takes steps towards improving the nested UI. There are no visual changes in this PR yet, but with the tiniest code change, we can make it so nested blocks are not indented, simply to make room for their side UI: I will tackle that further in a separate PR that intends to create a better nested UI, but this refactor will make it much easier. Due to all of this I have had to tweak the way we do responsive UI, which is now more resilient. There were some edgecases between the small and medium breakpoint where the block-toolbar would cause wide elements to generate a horizontal scrollbar. Due to the simpler Renamed a couple of variables to be more descriptive, and added some comments and section separators to the CSS files, to better document what's going on. The insert-between line positioning is now simpler and more code-readable as it uses variables. Changed the way borders are painted around the selected block, so now these also use negative margins, making the actual block dimensions more true. The original purpose of this PR was to allow margins to collapse. Per code feedback, I decided to postpone that until later, as this particular refactor was necessary to get in first. So what was removed? Well, I added back the CSS to disallow margins to collapse. I fully intend to revisit this, but given the refactors in this branch, I can do that separately as it requires somewhat major surgery of block.js, see
But the todo list for that future PR is now much simpler, as it only has to do the block.js surgery add a new class for the div that will hold the selected block outline, and deal with vertical margins. No responsive shenanigans, no horizontal margins/paddings work. |
@@ -1,46 +1,47 @@ | |||
.editor-block-list__layout { | |||
// prevent collapsing margins on container | |||
padding-top: .1px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing any effective changes of removing this style. What is this collapsing into?
Also noting that .editor-block-list__layout
is wrapping a nested container, so these styles should apply there as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Back when the initial purpose of this PR was to allow margins to collapse, I could swear there was an issue with the columns block and adjecent blocks that was fixed by adding this rule.
However since that's been pulled out, for now I'm going to also remove this rule. Since I hope to revisit collapsing margins in a future PR, I can always restore the rule if I recall what it did.
// make room in the main content column for the side UI | ||
// the side UI uses negative margins to position itself so as to not affect the block width | ||
@include break-small() { | ||
padding-left: $block-side-ui-padding; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also noting that
.editor-block-list__layout
is wrapping a nested container, so these styles should apply there as well.
Speaking of... this is probably where the regression I described is introduced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it without touching this rule. This is part of the refactor, and it should, for now, bleed into the columns block intentionally.
I fixed the appender by making it more like a block, when in fact it's actually a button.
> .editor-block-settings-menu { | ||
right: $block-mover-margin; | ||
.editor-block-settings-menu { | ||
right: -$block-mover-margin -$block-padding + 2px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Operators get a bit confusing here; one's a negation, the other's a subtraction, but they're applied the same (where it could be confused as a shorthand syntax e.g. margin: -5px -10px
).
At its most verbose, I could see this expressed as: #{ -1 * $block-mover-margin } - $block-padding + 2px
(Not really a suggestion, but it's how I'd express it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This rule worked:
right: #{ -1 * $block-mover-margin - $block-padding + 2px };
Does that address your concern?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that address your concern?
Sure 👍 Assumes one remembers their order of operations though. Tend to paranthesize on mixed operators.
right: #{ ( -1 * $block-mover-margin ) - $block-padding + 2px };
> .editor-block-mover { | ||
|
||
/** | ||
* Left and right side UI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Tab here after comment asterisk should be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
@@ -376,7 +426,7 @@ | |||
|
|||
.editor-block-list__insertion-point-indicator { | |||
position: absolute; | |||
top: 16px; | |||
top: $block-padding - 1px; // Half the empty space between two blocks, minus the 2px height |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Tab before comment should be a space.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 👍
Addressed the remaining feedback. |
ab7b274
to
71efa33
Compare
Rebased, and pushed a few fixes. Would be nice to get this in soon. |
onKeyDown={ onAppend } | ||
value={ showPrompt ? __( 'Write your story' ) : '' } | ||
/> | ||
<div className="editor-default-block-appender__content"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are failing snapshot tests because of the change in markup. Is this nesting really necessary though? Could we apply padding-top
to the parent .editor-default-block-appender
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch. I could swear it was necessary to make this change, so the hovered style matched what the end paragraph would be, but a later change must've made it unnecessary because reverting this fixes it.
* Selected Block style | ||
*/ | ||
|
||
.editor-block-list__block-edit { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these styles need to be nested, or can they be defined as a separate top-level style block?
Edit: I guess it has some benefit to colocation of &.is-selected
styles. Could also be separate, then targeting .editor-block-list__block.is-selected & {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not completely sure what you mean here, did you prefer the hover, and selected styles were moved out of .editor-block-list__layout .editor-block-list__block
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.editor-block-list__block-edit
- is unique, so it could be moved out of .editor-block-list__layout
and everything should still work.
In case of:
// focused block-style
&.is-selected .editor-block-list__block-edit:before {
outline: 1px solid $light-gray-500;
}
it would be more trickly, because of the dependency on the two parent classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After Grzegorz' explanation (thank you), and further thinking about this, I understand your initial point here, but also reached your same notion that the colocation of the selected blocks styles is beneficial even though the code is slightly less optimal.
In that vein, I think the entire block-list could be cleaned up a bit, there's a lot bunched together in a less than ideal way. But it feels like that might be good to do separately, and get this PR in as is, especially given that this file is likely to be revisited soon with nested UI improvements and collapsing margins.
Alternately we can move those rules out, as you suggest — it will just put them pretty far down the file as the .editor-block-list__block
is pretty big as is.
position: relative; | ||
|
||
&:before { | ||
z-index: z-index( '.editor-block-list__block:before' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should rename the z-index
key for the updated selector path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Good call. An ulterior motive of this PR is to start work on the nested UI. That means revisiting all the side UI, including on fullwide images. I'll take a look at the movers on wide/mobile, thanks. |
nested UI is definitely going to be a fun challenge, I have many ideas, but I mean to tackle them next. A plan b is to scroll the buttons horizontally when there's not enough space. But that's still a plan B. Another question emerges: should the columns block even show multiple columns at the mobile breakpoint? In any case, I believe this is ready to go. Thanks for the review. |
This rebase is really gnarly. I've tried and failed once — I would appreciate help if one of you has a chance to see if you do better than me. Let me know if we should pair or something. |
@jasmussen To help in the future, I'd recommend rebasing often and squashing commits in the process, whenever sensible. Old branches with dozens of commits are inevitably going to be frustrating to rebase, particularly when you're resolving conflicts for changes which no longer exist in later states of the branch. What I might suggest at this point is to squash on the merge base (where branch diverged from master):
You should have no conflicts in squashing here. You're just flattening commits a bit (or entirely to a single commit, if you choose). Then you can proceed to rebase again to master and have fewer individual commits to deal with. |
Ah that's a great tip. I'll try a good squash. For some reason I thought rebasing was the same. |
Rebasing alone is just moving the branch point from one commit to another (typically the current state of master). It then proceeds to apply all of the same commits, where obviously some conflicts may occur. Rebasing interactively gives you the opportunity to rewrite the commit history, including squash/fixup, dropping commits altogether, or even rearranging the order in which they're to be applied. So after squashing on the branch point to first reduce the number of commits, you should have an easier time making sense of conflict resolution of a subsequent rebase against master. I speak having been in similar nightmarish situations with ugly rebases 😆 |
yes, squash and rebase works like a charm. Otherwise, you have to apply the same changes over and over again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jasmussen great news is that your last commit fixed my two comments which cover 3 separate issues:
Great one 👍 I think it looks solid enough to proceed. Let's rebase and merge unless @aduth discovers any blockers :)
Quick note: Please be aware of the theme variable $content_width has been discussed a few times in core and it's seen as something "legacy", as it doesn't make much sense in a responsive era. It still make sense just for embeds, maybe. Some references: |
d816fd1
to
3ca0ed2
Compare
3ca0ed2
to
1d5367f
Compare
Okay, I'm pretty sure this is properly rebased now. But because I had to do a bunch of changes as a result of the recent appender/quick-block shortcuts, I'd like a new sanity check that things work as expected. That is — no need to test everything surrounding images, alignments, toolbars, all that — specifically test issues surrounding empty paragraph blocks, the side inserter, the quick block links on the right. Note that there are separate rules and behaviors for the appender — the inserter at the bottom, and the empty paragraph block. That is, these rules are mostly merged and should behave the same now, but just remember to test both. Thank you, and CC: @mtias to verify the fading effects work as intended. |
This PR is not ready to merge, and is still mostly in the proof of concept stage. What this PR does, is change how we render blocks, toolbars, and selected block borders. We now let the natural block margin shine through the chrome, and arrange blocks on the page. This also means we allow those very same margins to collapse according to standard web rules, which you can read about here: https://css-tricks.com/what-you-should-know-about-collapsing-margins/ Why? Because this is a more accurate representation of the natural structure of web content. The chief benefit is that once we start loading theme stylesheets into the editor, the standard margins applied by the theme author should map 1:1 to the structure of content inside Gutenberg, whereas right now in master special rules would have to be written for it to visually match, as in master we space blocks using _paddings_ instead of _margins_. Another benefit of this refactor is that it takes the block-padding math mostly out of the block UI, which will benefit nested block UI. There's still an artificial padding left and right to enable hovering to get to the movers and ellipsis, but we are many steps closer to that being easily addressed with a negative margin, which should fix nested blocks also. I had hoped that content adjecent to content in the new columns block would also magically collapse, but we probably need a special case for that block as according to the spec, CSS grid based elements don't allow collapsing margins: https://stackoverflow.com/questions/18963421/wanted-css-grid-system-and-collapsing-margins The next steps for this PR is lots of testing. There are many little UI glitches and positioning math that's just slightly off, which we'd need to fix before a merge. But most importantly it's important to test lots of edgecases, and verify that everything still works as intended.
More fixes to come.
More work to do still.
Would like to optimize the CSS a bit further.
f02f759
to
e87bb0b
Compare
Got a 👍 👍 in chat. Merging. |
This PR has been rejiggered to be a block margin refactor. It was previously a larger PR to allow margins to collapse, which would still be good to revisit. But keeping things slightly smaller, this is a step in that direction. Nested UI improvements and collapsing UI are next steps on this path.
GIF:
Original text:
What this PR does, is change how we render blocks, toolbars, and selected block borders. We now let the natural block margin shine through the chrome, and arrange blocks on the page. This also means we allow those very same margins to collapse according to standard web rules, which you can read about here: https://css-tricks.com/what-you-should-know-about-collapsing-margins/
Why?
Because this is a more accurate representation of the natural structure of web content. The chief benefit is that once we start loading theme stylesheets into the editor, the standard margins applied by the theme author should map 1:1 to the structure of content inside Gutenberg, whereas right now in master special rules would have to be written for it to visually match, as in master we space blocks using paddings instead of margins.
Another benefit of this refactor is that it takes the block-padding math mostly out of the block UI, which will benefit nested block UI. There's still an artificial padding left and right to enable hovering to get to the movers and ellipsis, but we are many steps closer to that being easily addressed with a negative margin, which should fix nested blocks also.
I had hoped that content adjecent to content in the new columns block would also magically collapse, but we probably need a special case for that block as according to the spec, CSS grid based elements don't allow collapsing margins: https://stackoverflow.com/questions/18963421/wanted-css-grid-system-and-collapsing-margins
The next steps for this PR is lots of testing. There are many little UI glitches and positioning math that's just slightly off, which we'd need to fix before a merge. But most importantly it's important to test lots of edgecases, and verify that everything still works as intended.