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

Remove inserter inside multi-selection #3246

Merged
merged 5 commits into from Nov 15, 2017

Conversation

Projects
None yet
3 participants
@iseulde
Member

iseulde commented Oct 30, 2017

Description

Fixes #3076.

How Has This Been Tested?

Make a multi block selection. Make sure there are inserters around the block (visible by hover), but not inside the block.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.

@iseulde iseulde requested review from mtias and aduth Oct 30, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 30, 2017

Codecov Report

Merging #3246 into master will increase coverage by <.01%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3246      +/-   ##
==========================================
+ Coverage   34.65%   34.65%   +<.01%     
==========================================
  Files         260      260              
  Lines        6741     6752      +11     
  Branches     1220     1225       +5     
==========================================
+ Hits         2336     2340       +4     
- Misses       3719     3723       +4     
- Partials      686      689       +3
Impacted Files Coverage Δ
editor/modes/visual-editor/sibling-inserter.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/selectors.js 93.12% <80%> (-0.36%) ⬇️

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 3f31557...62875b0. Read the comment docs.

codecov bot commented Oct 30, 2017

Codecov Report

Merging #3246 into master will increase coverage by <.01%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3246      +/-   ##
==========================================
+ Coverage   34.65%   34.65%   +<.01%     
==========================================
  Files         260      260              
  Lines        6741     6752      +11     
  Branches     1220     1225       +5     
==========================================
+ Hits         2336     2340       +4     
- Misses       3719     3723       +4     
- Partials      686      689       +3
Impacted Files Coverage Δ
editor/modes/visual-editor/sibling-inserter.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/selectors.js 93.12% <80%> (-0.36%) ⬇️

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 3f31557...62875b0. Read the comment docs.

@aduth

Elsewhere I've been combatting frequent re-renders of this component where the common culprit is the multi-selection props (see #3247, #3170). I think we would be better served, both for intent and performance, to have more of a separation between multi-selection and block list rendering.

As it impacts here, I wonder if it should be the responsibility of the sibling inserter to determine if it should be visible (or return null) depending on if the insertIndex is within range of the current multi-selection?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Oct 30, 2017

Member

@aduth Works for me too. Should we then pass down the multi selection props, or should it get all that itself?

Member

iseulde commented Oct 30, 2017

@aduth Works for me too. Should we then pass down the multi selection props, or should it get all that itself?

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Oct 30, 2017

Member

(Because the props are already present here.)

Member

iseulde commented Oct 30, 2017

(Because the props are already present here.)

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 30, 2017

Member

It should get itself, since this will trigger a render only on the individual component when the prop changes.

Member

aduth commented Oct 30, 2017

It should get itself, since this will trigger a render only on the individual component when the prop changes.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 30, 2017

Member

(Because the props are already present here.)

This is the issue though, since a change in reference value on multiSelectedBlocks and multiSelectedBlockUids causes a re-render on the full block listing.

Member

aduth commented Oct 30, 2017

(Because the props are already present here.)

This is the issue though, since a change in reference value on multiSelectedBlocks and multiSelectedBlockUids causes a re-render on the full block listing.

@iseulde

This comment has been minimized.

Show comment
Hide comment
@iseulde

iseulde Nov 2, 2017

Member

@aduth I'm not sure how we'd avoid that though. Do you have any ideas?

Member

iseulde commented Nov 2, 2017

@aduth I'm not sure how we'd avoid that though. Do you have any ideas?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Nov 6, 2017

Member

In this case I would suggest passing as a boolean prop into <VisualEditorSiblingInserter /> via connect, perhaps the result of a new selector isBlockWithinMultiSelect( state, uid ), and short-cutting the render (return null;) if truthy. Maybe a different selector name if the logic needs to be "within, except the last index".

In a normal case I wouldn't be too bothered one way or the other whether this logic is controlled by the list component or the child itself, but since inserter is otherwise a simple component and the block list component is excessively complex, it could help readability to delegate this behavior to the child.

In this specific circumstance, there's probably not much of a performance difference, only because we already have multiSelectedBlockUids available; the point might be to not have this prop passed in, since without memoization passing transformed array props will trigger a render by the new reference. So in general the advice is:

  • Pass simple props
  • Memoize if you can't (but this is a crutch)
  • It can be easier if you move to the connect logic of the child component, e.g. the simple boolean passed into VisualEditorSiblingInserter
Member

aduth commented Nov 6, 2017

In this case I would suggest passing as a boolean prop into <VisualEditorSiblingInserter /> via connect, perhaps the result of a new selector isBlockWithinMultiSelect( state, uid ), and short-cutting the render (return null;) if truthy. Maybe a different selector name if the logic needs to be "within, except the last index".

In a normal case I wouldn't be too bothered one way or the other whether this logic is controlled by the list component or the child itself, but since inserter is otherwise a simple component and the block list component is excessively complex, it could help readability to delegate this behavior to the child.

In this specific circumstance, there's probably not much of a performance difference, only because we already have multiSelectedBlockUids available; the point might be to not have this prop passed in, since without memoization passing transformed array props will trigger a render by the new reference. So in general the advice is:

  • Pass simple props
  • Memoize if you can't (but this is a crutch)
  • It can be easier if you move to the connect logic of the child component, e.g. the simple boolean passed into VisualEditorSiblingInserter
@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Nov 15, 2017

Contributor

Rebased and refactored according to review suggestions. I only now saw the reference to #3485; hopefully the two fixes can fix together nicely.

Contributor

mcsf commented Nov 15, 2017

Rebased and refactored according to review suggestions. I only now saw the reference to #3485; hopefully the two fixes can fix together nicely.

@aduth

aduth approved these changes Nov 15, 2017

LGTM 👍

@mcsf mcsf merged commit b56f09c into master Nov 15, 2017

3 checks passed

codecov/project 34.65% (+<.01%) compared to 3f31557
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@mcsf mcsf deleted the fix/inserter-inside-selection branch Nov 15, 2017

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