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

Refactor selectors used in copy-handler #11844

Merged
merged 3 commits into from Nov 14, 2018

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Nov 14, 2018

Extracted form #11811

In this PR, I avoid using the not-so performant selectors getSelectedBlock and getMultiSelectedBlocks in the withSelect HoC and I replace them with getSelectedBlockClientId and a getBlocks selectors only used in the block handler.

@youknowriad youknowriad added this to the 4.4 milestone Nov 14, 2018

@youknowriad youknowriad self-assigned this Nov 14, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Nov 14, 2018

// We only care about this value when the copy is performed
// We call it dynamically in the event handler to avoid unnecessary re-renders.
getBlocks,

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

I've used the same technique used in the other PR (cc @gziolo @aduth), it's becoming a pattern, we might want to find a more elegant solution in a follow-up PR. (withDispatch access to select somehow)

This comment has been minimized.

@gziolo

gziolo Nov 14, 2018

Member

As far as I remember withDispatch wraps those handlers with proxies to ensure they don’t update on every prop change. So you can’t execute select outside of those handlers as they will become immediately stale. We will have to document it properly to avoid any bugs. It is advanced usage which is necessary mostly in core, so it might be okey to attack it this way.

This comment has been minimized.

@gziolo

gziolo Nov 14, 2018

Member

I will open PR and let's discuss it there.

@gziolo

There is one place where I'm not sure whether the logic is exactly the same.

@@ -26,18 +26,18 @@ class CopyHandler extends Component {
}
onCopy( event ) {
const { multiSelectedBlocks, selectedBlock } = this.props;
const { selectedBlockClientIds, getBlocks } = this.props;

This comment has been minimized.

@gziolo

gziolo Nov 14, 2018

Member
Suggested change Beta
const { selectedBlockClientIds, getBlocks } = this.props;
const { selectedBlockClientIds = [], getBlocks } = this.props;

would it be possible to use to simplify next conditional check?

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

Actually it's an array in all cases, I'll just remove the first check.

@@ -46,12 +46,12 @@ class CopyHandler extends Component {
}
onCut( event ) {
const { multiSelectedBlockClientIds } = this.props;
const { selectedBlockClientIds } = this.props;

This comment has been minimized.

@gziolo

gziolo Nov 14, 2018

Member

The same as above with defaulting to an empty array.

this.onCopy( event );
if ( multiSelectedBlockClientIds.length ) {
this.props.onRemove( multiSelectedBlockClientIds );
if ( selectedBlockClientIds && selectedBlockClientIds.length ) {

This comment has been minimized.

@gziolo

gziolo Nov 14, 2018

Member
const selectedBlockClientIds = selectedBlockClientId ? [ selectedBlockClientId ] : getMultiSelectedBlockClientIds();

Should there be a check whethere there is more than one id in here?

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

You're right 👍

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

Actually, I'd add a boolean flag hasMultiSelection it's more secure

@youknowriad youknowriad force-pushed the refactor/copy-handler branch from b8dde83 to 875d911 Nov 14, 2018

@gziolo

gziolo approved these changes Nov 14, 2018

Everything was addressed, thanks. You can 🚢

@youknowriad youknowriad merged commit b2e5ae7 into master Nov 14, 2018

1 check passed

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

@youknowriad youknowriad deleted the refactor/copy-handler branch Nov 14, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

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