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: Replace connect usage with withSelect/withDispatch (1st round) #6142

Merged
merged 2 commits into from Apr 13, 2018

Conversation

Projects
None yet
3 participants
@youknowriad
Contributor

youknowriad commented Apr 12, 2018

Just getting rid of some legacy. and to avoid huge PRs, I'm updating 20 components in this first round. There's 45 remaining usage of connect in the editor module.

Testing instructions

Check that:

  • autosaves triggers
  • the block movers work
  • the block toolbar work
  • the block settings menu work
  • the in between inserter works
  • the default appender works.
  • the block inspector works
  • multiselection works
  • transformations 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 12, 2018

@youknowriad youknowriad requested a review from WordPress/gutenberg-core Apr 12, 2018

@gziolo

I left my comments, it all looks great. Happy to see how it looks after all changes get applied. In my opinion data module plays nicely!

I haven't found time to test. I can do it tomorrow.

isDirty: isEditedPostDirty(),
isSaveable: isEditedPostSaveable(),
};
} ),

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Nit: No complaints from Eslint? Interesting. ) should align with withSelect :)

import TextareaAutosize from 'react-autosize-textarea';
import { isEqual } from 'lodash';

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Nice catch 👍

block: getBlock( state, ownProps.uid ),
} ),
{
export default compose( [

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

@aduth, should we add Eslint rule to enforce arrays for compose? I always forgot to add it :)

This comment has been minimized.

@aduth

aduth Apr 13, 2018

Member

@aduth, should we add Eslint rule to enforce arrays for compose? I always forgot to add it :)

Meh.

"Developer preference" 😃

)
return {
showInsertionPoint: (
isBlockInsertionPointVisible() &&

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Can we extract inline variable for this one? It's super hard to follow.

} = dispatch( 'core/editor' );
return {
onStartMultiSelect: startMultiSelect,

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Nice 👍

return {
sharedBlock: isSharedBlock( block ) ? getSharedBlock( state, block.attributes.ref ) : null,
sharedBlock: block && isSharedBlock( block ) ? getSharedBlock( block.attributes.ref ) : null,

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Interesting improvement, makes sense :)

// TODO: Make this a <Confirm /> component or similar
// eslint-disable-next-line no-alert
const hasConfirmed = window.confirm( __(
'Are you sure you want to delete this Shared Block?\n\n' +
const hasConfirmed = window.confirm( __(

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Nit: Something went wrong with the formatting here.

};
withSelect( ( select, ownProps ) => {
return {
blocks: ownProps.uids.map( ( uid ) => select( 'core/editor' ).getBlock( uid ) ),

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Not related to your change, but map usage in here looks suspicious. It probably rerenders too often.

};
}
export const moveBlocksDown = createOnMove( 'MOVE_BLOCKS_DOWN' );

This comment has been minimized.

@gziolo

gziolo Apr 12, 2018

Member

Partial application ❤️

@gziolo

gziolo approved these changes Apr 13, 2018

Nice work, I haven't found any regressions. Let's ship it :shipit:

@youknowriad youknowriad merged commit 751245a into master Apr 13, 2018

2 checks passed

codecov/project 44.37% (-0.07%) compared to 520cb09
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/with-select-editor-1 branch Apr 13, 2018

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