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 Module: Use withSelect/withDispatch instead of connect (round 2) #6167

Merged
merged 1 commit into from Apr 16, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Apr 13, 2018

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 connect in the editor module.

Testing instructions

Check that:

  • the document title updates
  • the undo/redo buttons work
  • the editor notices show up
  • the inserter works
  • publishing/updating a post work
  • Changing the author/pending stats/post format/page attributes work/comment status work

I already tested everything, so it should be ok. I'm willing to merge this pretty quickly to avoid a lot of conflicts.

@youknowriad youknowriad self-assigned this Apr 13, 2018

@youknowriad youknowriad requested a review from gziolo Apr 13, 2018

hasRedo: select( 'core/editor' ).hasEditorRedo(),
} ) ),
withDispatch( ( dispatch ) => ( {
redo: () => dispatch( 'core/editor' ).redo(),

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 13, 2018

Member

Minor: Can we just write: redo: dispatch( 'core/editor' ).redo?

This comment has been minimized.

@youknowriad

youknowriad Apr 13, 2018

Contributor

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.

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 13, 2018

Member

It makes sense, thank you for clarifying @youknowriad 👍

isEditedPostSaveable,
} = select( 'core/editor' );
return {
postId: getCurrentPost().id,

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 13, 2018

Member

Minor: Maybe we can use getCurrentPostId()?

const applyWithEditorSettings = withEditorSettings(
( settings ) => ( {
} ) ),
withEditorSettings( ( settings ) => ( {

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 13, 2018

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Apr 13, 2018

Contributor

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

This comment has been minimized.

@jorgefilipecosta

jorgefilipecosta Apr 13, 2018

Member

Yes valid point, I had only seen the first case so it did not occur to me the second case could also be valid.

@jorgefilipecosta

Nice work handling this change 👍 Things seem to work fine in my tests!

@youknowriad youknowriad merged commit d5fc1cf into master Apr 16, 2018

2 checks passed

codecov/project 44.13% (-0.23%) compared to 0d2cce9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/with-select-editor-2 branch Apr 16, 2018

@gziolo

This comment has been minimized.

Member

gziolo commented Apr 16, 2018

🎉

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