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

Editor: Extract BlockList as reusable component #3546

Merged
merged 2 commits into from Nov 21, 2017

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented Nov 18, 2017

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.

@aduth aduth added the Framework label Nov 18, 2017

@aduth aduth requested a review from youknowriad Nov 18, 2017

@@ -361,7 +361,7 @@ export default class Editable extends Component {
// Find the parent "relative" positioned container
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' );

This comment has been minimized.

@ephox-mogran

ephox-mogran Nov 20, 2017

Contributor

Is this a good opportunity to stop using a hard-coded selector for this? What are some other options?

@ephox-mogran

ephox-mogran Nov 20, 2017

Contributor

Is this a good opportunity to stop using a hard-coded selector for this? What are some other options?

This comment has been minimized.

@ephox-mogran

ephox-mogran Nov 20, 2017

Contributor

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).

@ephox-mogran

ephox-mogran Nov 20, 2017

Contributor

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).

This comment has been minimized.

@aduth

aduth Nov 20, 2017

Member

Is this a good opportunity to stop using a hard-coded selector for this? What are some other options?
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).

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.

@aduth

aduth Nov 20, 2017

Member

Is this a good opportunity to stop using a hard-coded selector for this? What are some other options?
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).

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.

This comment has been minimized.

@aduth

aduth Nov 20, 2017

Member

Although given some of the immediate concerns of moving this as raised in #3546 (comment), maybe it is a prime time 😕

@aduth

aduth Nov 20, 2017

Member

Although given some of the immediate concerns of moving this as raised in #3546 (comment), maybe it is a prime time 😕

@@ -56,7 +56,7 @@ import {
const { BACKSPACE, ESCAPE, DELETE, ENTER, UP, RIGHT, DOWN, LEFT } = keycodes;
class VisualEditorBlock extends Component {

This comment has been minimized.

@youknowriad

youknowriad Nov 20, 2017

Contributor

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.

@youknowriad

youknowriad Nov 20, 2017

Contributor

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.

This comment has been minimized.

@aduth

aduth Nov 20, 2017

Member

In d8be96b I reimplemented the behavior to a generic "find closest scrollable container".

@aduth

aduth Nov 20, 2017

Member

In d8be96b I reimplemented the behavior to a generic "find closest scrollable container".

@youknowriad

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 🎉

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 20, 2017

Contributor

related #2761

Contributor

youknowriad commented Nov 20, 2017

related #2761

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 20, 2017

Contributor

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.

Contributor

youknowriad commented Nov 20, 2017

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.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 20, 2017

Codecov Report

Merging #3546 into master will increase coverage by 0.64%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
.../components/block-list/block-contextual-toolbar.js 0% <ø> (ø)
editor/components/block-list/multi-controls.js 0% <ø> (ø)
editor/components/block-list/block-drop-zone.js 0% <ø> (ø)
...tor/components/block-list/invalid-block-warning.js 0% <ø> (ø)
...ditor/components/block-list/block-crash-warning.js 50% <ø> (ø)
editor/components/block-list/block-html.js 0% <ø> (ø)
blocks/editable/index.js 10.23% <ø> (ø) ⬆️
...itor/components/block-list/block-crash-boundary.js 0% <ø> (ø)
editor/components/block-list/index.js 0% <ø> (ø)
editor/components/block-list/block.js 0.75% <0%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d50d76...d8be96b. Read the comment docs.

codecov bot commented Nov 20, 2017

Codecov Report

Merging #3546 into master will increase coverage by 0.64%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
.../components/block-list/block-contextual-toolbar.js 0% <ø> (ø)
editor/components/block-list/multi-controls.js 0% <ø> (ø)
editor/components/block-list/block-drop-zone.js 0% <ø> (ø)
...tor/components/block-list/invalid-block-warning.js 0% <ø> (ø)
...ditor/components/block-list/block-crash-warning.js 50% <ø> (ø)
editor/components/block-list/block-html.js 0% <ø> (ø)
blocks/editable/index.js 10.23% <ø> (ø) ⬆️
...itor/components/block-list/block-crash-boundary.js 0% <ø> (ø)
editor/components/block-list/index.js 0% <ø> (ø)
editor/components/block-list/block.js 0.75% <0%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d50d76...d8be96b. Read the comment docs.

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 20, 2017

Codecov Report

Merging #3546 into master will increase coverage by 0.64%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
editor/components/block-list/block-drop-zone.js 0% <ø> (ø)
.../components/block-list/block-contextual-toolbar.js 0% <ø> (ø)
editor/components/block-list/multi-controls.js 0% <ø> (ø)
...itor/components/block-list/block-crash-boundary.js 0% <ø> (ø)
editor/components/block-list/index.js 0% <ø> (ø)
...ditor/components/block-list/block-crash-warning.js 50% <ø> (ø)
blocks/editable/index.js 10.23% <ø> (ø) ⬆️
editor/components/block-list/block-html.js 0% <ø> (ø)
...tor/components/block-list/invalid-block-warning.js 0% <ø> (ø)
editor/components/writing-flow/index.js 1.47% <0%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 359f11f...ca43cb9. Read the comment docs.

codecov bot commented Nov 20, 2017

Codecov Report

Merging #3546 into master will increase coverage by 0.64%.
The diff coverage is 0%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
editor/components/block-list/block-drop-zone.js 0% <ø> (ø)
.../components/block-list/block-contextual-toolbar.js 0% <ø> (ø)
editor/components/block-list/multi-controls.js 0% <ø> (ø)
...itor/components/block-list/block-crash-boundary.js 0% <ø> (ø)
editor/components/block-list/index.js 0% <ø> (ø)
...ditor/components/block-list/block-crash-warning.js 50% <ø> (ø)
blocks/editable/index.js 10.23% <ø> (ø) ⬆️
editor/components/block-list/block-html.js 0% <ø> (ø)
...tor/components/block-list/invalid-block-warning.js 0% <ø> (ø)
editor/components/writing-flow/index.js 1.47% <0%> (ø) ⬆️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 359f11f...ca43cb9. Read the comment docs.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 20, 2017

Member

If I understand properly, this could change over time, which means we can't call the function in the constructor only. Say the block list is small and then it becomes long enought at some point?

This is a good point. In the rebased d6fb66d I call to retrieve the scroll container after the block has been moved.

Member

aduth commented Nov 20, 2017

If I understand properly, this could change over time, which means we can't call the function in the constructor only. Say the block list is small and then it becomes long enought at some point?

This is a good point. In the rebased d6fb66d I call to retrieve the scroll container after the block has been moved.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Nov 21, 2017

Contributor

closes #2959

Contributor

youknowriad commented Nov 21, 2017

closes #2959

@aduth aduth merged commit c52e0b4 into master Nov 21, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the move/block-list branch Nov 21, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 21, 2017

Member

Noting that this could break some styles with custom blocks which relied on the .editor-block-list__block[data-type] convention for applying styles. Ideally we wouldn't be encouraging this dependency on block list classes anyways, and assume that the block's own applied classes are unique enough for styling.

Member

aduth commented Nov 21, 2017

Noting that this could break some styles with custom blocks which relied on the .editor-block-list__block[data-type] convention for applying styles. Ideally we wouldn't be encouraging this dependency on block list classes anyways, and assume that the block's own applied classes are unique enough for styling.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 21, 2017

Member

Looks like between-inserter style regressed a bit from the changes here. Currently investigating.

Edit: See #3597

Member

aduth commented Nov 21, 2017

Looks like between-inserter style regressed a bit from the changes here. Currently investigating.

Edit: See #3597

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