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

Fix getSelectedBlockClientId selector #12214

Merged
merged 5 commits into from Nov 22, 2018

Conversation

Projects
None yet
5 participants
@nosolosw
Member

nosolosw commented Nov 22, 2018

Addresses #12002
Superseedes #12209 and #12052 that tried to fix this only for the BlockSettingsMenu component.

At the moment, we don't have a way to clear or update block selection for UNDO / REDO actions. This is problematic for every client that uses getSelectedBlockClientId because they may end up with a block that no longer exist in the editor.

By inverting the dependencies between getSelectedBlockClientId and getSelectedBlock we make sure we always return a valid clientId in getSelectedBlockClientId.

We still have to fix the UNDO/REDO problem, but this patch gives us time to think while the external API works as expected.

Fix getSelectedBlockClientId selector
At the moment, we don't have a way to clear or update
block selection for UNDO / REDO actions.

By inverting the dependencies between getSelectedBlockClientId
and getSelectedBlock we make sure we always return
a valid clientId in getSelectedBlockClientId.

We still have to fix the UNDO/REDO problem internally,
but, in the meantime, this patch fixes the API.
Show resolved Hide resolved packages/editor/src/store/selectors.js Outdated
Show resolved Hide resolved packages/editor/src/store/test/selectors.js Outdated
Show resolved Hide resolved packages/editor/src/store/selectors.js Outdated
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 22, 2018

they may end up with a block that no longer exist in the editor.

This makes me thing the fix should be done in the reducer ideally, whenever the block is removed and it's the currently selected block, reset the selection in the reducer.

@gziolo

This comment has been minimized.

Member

gziolo commented Nov 22, 2018

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 22, 2018

I'm pretty sure we're missing a case there as it's the only way getSelectedBlock can be empty but not getSelectedBlockClientId. Maybe another action.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 22, 2018

Or I guess it's because this reducer is not inside the "history" :) mmm interesting issue. While we can fix it by tweaking the selector to check for existence first. I wonder if the selection reducer should be part of the history somehow. cc @aduth

@nosolosw

This comment has been minimized.

Member

nosolosw commented Nov 22, 2018

@gziolo @youknowriad for reference, take a look at this comment #12002 (comment) I agree this should be fixed in the reducer, but I've hit some deadends in every approach I've tried so far.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 22, 2018

This bandaid fix could be something like start === end && start && !! state.editor.blocks.order[ start ] ? start : null;

@nosolosw

This comment has been minimized.

Member

nosolosw commented Nov 22, 2018

Pushed a more performant variant of this. And less code!

@youknowriad

I'm fine with this fix for now.

Separately, I'd love to see if we can move the blockSelection reducer inside the withHistory HoR as I also think it would fix the issue but not sure yet about the side-effects.

@youknowriad youknowriad merged commit 0298cac into master Nov 22, 2018

1 check passed

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

@youknowriad youknowriad deleted the fix/getselectedblockclientid branch Nov 22, 2018

@aduth

This comment has been minimized.

Member

aduth commented Nov 26, 2018

Separately, I'd love to see if we can move the blockSelection reducer inside the withHistory HoR as I also think it would fix the issue but not sure yet about the side-effects.

Is there an issue for this?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 26, 2018

Just created this #12327 thanks @aduth for the reminder.

// We need to check the block exists because the current state.blockSelection reducer
// doesn't take into account the UNDO / REDO actions to update selection.
// To be removed when that's fixed.
return start && start === end && !! state.editor.present.blocks.byClientId[ start ] ? start : null;

This comment has been minimized.

@mcsf

mcsf Nov 29, 2018

Contributor

Minor / more of a recommendation for the future: I get uneasy when I see chains of mixed infix operators, and would prefer this written as ( a && a === b && !! c ) ? a : nulli.e. with the brackets around the ternary's condition.

youknowriad added a commit that referenced this pull request Nov 29, 2018

Fix getSelectedBlockClientId selector (#12214)
* Fix getSelectedBlockClientId selector

At the moment, we don't have a way to clear or update
block selection for UNDO / REDO actions.

By inverting the dependencies between getSelectedBlockClientId
and getSelectedBlock we make sure we always return
a valid clientId in getSelectedBlockClientId.

We still have to fix the UNDO/REDO problem internally,
but, in the meantime, this patch fixes the API.

* Fix tests

* Revert "Fix getSelectedBlockClientId selector"

This reverts commit cfa3066.

* Revert "Fix tests"

This reverts commit 60abeab.

* Use a more performant variant
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment