-
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
Editor: Use store to always have up to date list of format types #11483
Conversation
export default FormatEdit; | ||
export default withSelect( | ||
( select ) => { | ||
const { getFormatTypes } = select( 'core/rich-text' ); |
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 love this change and I think we should do something similar for the blocks too.
For example, instead of using a filter when registering block styles, keep track of block styles in a separate reducer of the blocks
store and use a selector. This way we can register block styles at any time.
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.
Yes, I agree that we should do the same for blocks and plugins. Always go through withSelect
to take advantage of its buil-in reactive nature.
@@ -5,8 +5,6 @@ export { charAt } from './char-at'; | |||
export { concat } from './concat'; | |||
export { create } from './create'; | |||
export { getActiveFormat } from './get-active-format'; | |||
export { getFormatType } from './get-format-type'; | |||
export { getFormatTypes } from './get-format-types'; |
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.
This is a breaking change, right? I think it's probably fine to just remove as I don't expect plugins using that (as it's very recent) but It thought I'd mention.
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.
It depends if we cherry-pick it for 4.2 release - it won't be a breaking change then. We discussed doing that with @iseulde.
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.
Sounds good to me to include in 4.2.
|
Also a more general problem with unregistering formats: how do we know how to serialise the format if it was registered when it was created, but no longer there on serialisation? |
I bet we don't do it anymore. We just skip all the changes that don't have a corresponding registered format type. The same applies to blocks. If we don't have a matching block type, we just treat it as a plain HTML snippet. |
@youknowriad can we still add it to 4.2 release somehow? :) |
Description
This PR refactors
RichText
component to usewithSelect
to get the list of format types. This ensures that every change to the list of format types is immediately reflected in the UI. Without this change, the user needed to perform an action which explicitly re-render toolbar controls (select other RichText or change the state of controls, etc).Given that
getFormatTypes
andgetFormatType
methods are no longer referenced outside of@wordpress/rich-text
package I made them private (they were never documented as public). This should also help to ensure that they are consumed through the data layer abstractions.How has this been tested?
npm test
RichText
(paragraph or quote, list, etc ..)Types of changes
Refactoring.
Checklist: