-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 RichText rerendering when it shouldn't #12161
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,8 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import memize from 'memize'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see this added as a dependency. |
||
|
||
/** | ||
* WordPress dependencies | ||
*/ | ||
|
@@ -115,6 +120,40 @@ function updateAnnotationsWithPositions( annotations, positions, { removeAnnotat | |
} ); | ||
} | ||
|
||
/** | ||
* Create prepareEditableTree memoized based on the annotation props. | ||
* | ||
* @param {Object} The props with annotations in them. | ||
* | ||
* @return {Function} The prepareEditableTree. | ||
*/ | ||
const createPrepareEditableTree = memize( ( props ) => { | ||
const { annotations } = props; | ||
|
||
return ( formats, text ) => { | ||
if ( annotations.length === 0 ) { | ||
return formats; | ||
} | ||
|
||
let record = { formats, text }; | ||
record = applyAnnotations( record, annotations ); | ||
return record.formats; | ||
}; | ||
} ); | ||
|
||
/** | ||
* Returns the annotations as a props object. Memoized to prevent re-renders. | ||
* | ||
* @param {Array} The annotations to put in the object. | ||
* | ||
* @return {Object} The annotations props object. | ||
*/ | ||
const getAnnotationObject = memize( ( annotations ) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of memoizing function, I think it would be better for the That said, we're kind of running out of time, so I'm not considering this as blocking at the moment as it's a forward-compatible change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, this sounds better. |
||
return { | ||
annotations, | ||
}; | ||
} ); | ||
|
||
export const annotation = { | ||
name: FORMAT_NAME, | ||
title: __( 'Annotation' ), | ||
|
@@ -128,21 +167,9 @@ export const annotation = { | |
return null; | ||
}, | ||
__experimentalGetPropsForEditableTreePreparation( select, { richTextIdentifier, blockClientId } ) { | ||
return { | ||
annotations: select( STORE_KEY ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ), | ||
}; | ||
}, | ||
__experimentalCreatePrepareEditableTree( { annotations } ) { | ||
return ( formats, text ) => { | ||
if ( annotations.length === 0 ) { | ||
return formats; | ||
} | ||
|
||
let record = { formats, text }; | ||
record = applyAnnotations( record, annotations ); | ||
return record.formats; | ||
}; | ||
return getAnnotationObject( select( STORE_KEY ).__experimentalGetAnnotationsForRichText( blockClientId, richTextIdentifier ) ); | ||
}, | ||
__experimentalCreatePrepareEditableTree: createPrepareEditableTree, | ||
__experimentalGetPropsForEditableTreeChangeHandler( dispatch ) { | ||
return { | ||
removeAnnotation: dispatch( STORE_KEY ).__experimentalRemoveAnnotation, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we also need it on the server side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't, there's no script for memize
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, cool then :)