Skip to content
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

RichText: rewrite withFilters with hooks #19117

Merged
merged 16 commits into from
Dec 17, 2019
Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Dec 13, 2019

Description

Removes rich-text's dependency on the hooks package.

Currently some props are added for the annotation format types with WordPress hooks. This is unnecessary, because RichText has access to the format types. Additionally the addition of the props can be simplified with hooks and a single wrapper component.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

export { useDispatch } from './components/use-dispatch';
export {
useDispatch,
useDispatchWithMap as __unstableUseDispatchWithMap,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@youknowriad Just wanted to make you aware that I'm exporting useDispatchWithMap here (unstable API). It's needed for RichText because the format type needs access to dispatch to pick a store. I don't think there's any way around it, but maybe you have a better insight.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the format type needs access to dispatch to pick a store

Maybe, there's an alternative way to provide access to the hook in these types. Could you give a quick example about how we use dispatch in the formats?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure:

__experimentalGetPropsForEditableTreeChangeHandler( dispatch ) {
return {
removeAnnotation: dispatch( STORE_KEY ).__experimentalRemoveAnnotation,
updateAnnotationRange: dispatch( STORE_KEY ).__experimentalUpdateAnnotationRange,
};
},

@ellatrix ellatrix added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Code Quality Issues or PRs that relate to code quality labels Dec 13, 2019
@ellatrix
Copy link
Member Author

Did a performance check and the results vary with slightly better performance for this branch.

@ellatrix ellatrix merged commit 33cbd3b into master Dec 17, 2019
@ellatrix ellatrix deleted the try/rich-text-filters branch December 17, 2019 12:47
@hypest
Copy link
Contributor

hypest commented Dec 17, 2019

Headsup, formatTypes changes from this PR introduced a crash regression in the native mobile codepath. @mchowning is having more contect in this one (and is in the process of opening a gutenberg-mobile ticket to track. Edit: gutenberg-mobile ticket).

@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants