Editor: Extract BlockList as reusable component#3546
Conversation
| const container = this.props.inlineToolbar ? | ||
| this.editor.getBody().closest( '.blocks-editable' ) : | ||
| this.editor.getBody().closest( '.editor-visual-editor__block' ); | ||
| this.editor.getBody().closest( '.editor-block-list__block' ); |
There was a problem hiding this comment.
Is this a good opportunity to stop using a hard-coded selector for this? What are some other options?
There was a problem hiding this comment.
We could also use context and some HOCs to specify the component to use for anchoring directly as well (rather than relying on DOM hierarchy and selectors).
There was a problem hiding this comment.
Is this a good opportunity to stop using a hard-coded selector for this? What are some other options?
We could also usecontextand someHOCsto specify the component to use for anchoring directly as well (rather than relying on DOM hierarchy and selectors).
I definitely agree with the desire here, and hope to discourage these hard-coded selectors from being introduced in the first place (this pull request serving as evidence of the maintenance pain they cause). But I'd rather address these separate from the intent of this pull request.
There was a problem hiding this comment.
Although given some of the immediate concerns of moving this as raised in #3546 (comment), maybe it is a prime time 😕
| margin-left: auto; | ||
| margin-right: auto; | ||
| margin-bottom: $block-spacing; | ||
| max-width: $visual-editor-max-width + ( 2 * $block-mover-padding-visible ); |
There was a problem hiding this comment.
I think all these styles containing margin: auto and max-width and more globally all the styles positionning the components inside the PostEditor constraints should be kept outside of the editor/components directory.
That way, A layout inserting the BlockList component shouldn't have to think about resetting those to style itself. (Think P2 including BlockList).
At least, that's whay I did for all the other components extracted to this folder.
There was a problem hiding this comment.
Accounted for the margins and maximum widths in the rebased 87e417f.
| @@ -56,7 +56,7 @@ import { | |||
|
|
|||
| const { BACKSPACE, ESCAPE, DELETE, ENTER, UP, RIGHT, DOWN, LEFT } = keycodes; | |||
|
|
|||
| class VisualEditorBlock extends Component { | |||
There was a problem hiding this comment.
Making this reusable means we need to use a prop or something to get a selector for the parent scrollable. https://github.com/WordPress/gutenberg/pull/3546/files#diff-bc66d5701a2fea1111bb8f1bf0150910R96
It's not guaranteed to be the same in all layouts.
There was a problem hiding this comment.
In d8be96b I reimplemented the behavior to a generic "find closest scrollable container".
youknowriad
left a comment
There was a problem hiding this comment.
Just noticed that alongside WritingFlow this is the last component that needs to be moved to the editor/components folder. Hey we're getting there 🎉
|
related #2761 |
|
Not specific to this one because other components do so aswell, but we should avoid using all "preferences" in these components. I'm thinking of the "toolbar fixed or not" pref and "the most used blocks" pref. I'm thinking the "preferences" are layout specific. We could extract them in a separate PR. |
afed16d to
d8be96b
Compare
Codecov Report
@@ Coverage Diff @@
## master #3546 +/- ##
==========================================
+ Coverage 35.15% 35.79% +0.64%
==========================================
Files 267 267
Lines 6724 6612 -112
Branches 1218 1198 -20
==========================================
+ Hits 2364 2367 +3
+ Misses 3683 3588 -95
+ Partials 677 657 -20
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #3546 +/- ##
=========================================
+ Coverage 35.75% 36.4% +0.64%
=========================================
Files 267 267
Lines 6745 6634 -111
Branches 1221 1202 -19
=========================================
+ Hits 2412 2415 +3
+ Misses 3658 3563 -95
+ Partials 675 656 -19
Continue to review full report at Codecov.
|
d8be96b to
d6fb66d
Compare
This is a good point. In the rebased d6fb66d I call to retrieve the scroll container after the block has been moved. |
| // Preserve scroll prosition when block rearranged | ||
| if ( this.previousOffset ) { | ||
| this.editorLayout.scrollTop = this.editorLayout.scrollTop + | ||
| const scrollContainer = getScrollContainer( this.node ); |
There was a problem hiding this comment.
I have a small concern about this getting called in each block update. Let's try and see if we notice perf issues.
There was a problem hiding this comment.
I have a small concern about this getting called in each block update. Let's try and see if we notice perf issues.
This is very good to be concerned about, it shouldn't have been so generic, especially since window.getComputedStyle is a non-trivial operation. We can limit it to only the cases where a block has been moved by first checking this.previousOffset.
|
closes #2959 |
d6fb66d to
ca43cb9
Compare
|
Noting that this could break some styles with custom blocks which relied on the |
|
Looks like between-inserter style regressed a bit from the changes here. Currently investigating. Edit: See #3597 |
This pull request seeks to extract block list components from
VisualEditor, for purpose of reusability, particularly for managing nested block listing.Testing instructions:
Verify that there are no remaining lingering references to visual editor for what had previously targeted block list components (unfortunately there are many implicit dependencies, which should be minimized in future refactoring).
Verify that there is no regressions in block listing behavior.