Editor Module: Use withSelect/withDispatch instead of connect (round 2) #6167
Conversation
| hasRedo: select( 'core/editor' ).hasEditorRedo(), | ||
| } ) ), | ||
| withDispatch( ( dispatch ) => ( { | ||
| redo: () => dispatch( 'core/editor' ).redo(), |
There was a problem hiding this comment.
Minor: Can we just write: redo: dispatch( 'core/editor' ).redo?
There was a problem hiding this comment.
The reason I didn't do this is because we're attaching this directly to the onclick handler which means it receives the event as argument and I don't want it to be passed too the action creator. Even if in this particular case it doesn't do anything special since it just ignores all arguments, but I already had to fix a bug because of this exact same issue where we were passing an argument inadvertently.
There was a problem hiding this comment.
It makes sense, thank you for clarifying @youknowriad 👍
| isEditedPostSaveable, | ||
| } = select( 'core/editor' ); | ||
| return { | ||
| postId: getCurrentPost().id, |
There was a problem hiding this comment.
Minor: Maybe we can use getCurrentPostId()?
| const applyWithEditorSettings = withEditorSettings( | ||
| ( settings ) => ( { | ||
| } ) ), | ||
| withEditorSettings( ( settings ) => ( { |
There was a problem hiding this comment.
Minor: Normally I prefer to have withEditorSettings before withSelect and withDispatch as what we query from the store may one day depend on the editor settings but the editor settings don't depend on the store.
There was a problem hiding this comment.
Well, it depends sometimes you need the props (that can come from the store) to check or not a setting. But I guess it's less common
There was a problem hiding this comment.
Yes valid point, I had only seen the first case so it did not occur to me the second case could also be valid.
youknowriad
force-pushed
the
update/with-select-editor-2
branch
from
8e91d29 to
f7816ca
Compare
|
🎉 |
Just getting rid of some legacy. and to avoid huge PRs, I'm updating 20 components in this first round. There's 42 remaining usage of
connectin the editor module.Testing instructions
Check that:
I already tested everything, so it should be ok. I'm willing to merge this pretty quickly to avoid a lot of conflicts.