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

Update: Refactor paragraph block to be a functional component #18125

Conversation

jorgefilipecosta
Copy link
Member

Description

This PR updates the paragraph block to be a functional component.
We are introducing new hook based API's (e.g: the colors hook) and these API's can only be used if the component is functional.

How has this been tested?

I verified the paragraph still works as before.

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality labels Oct 27, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the update/refactor-paragraph-block-to-be-a-functional-component branch from a229709 to f453813 Compare October 28, 2019 17:46
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I started by commenting on one useMemo/useCallback but I noticed that it's a trend across all props and callbacks. I do wonder if we should use these two hooks in our refactorings from component classes to hooks. I understand that sometimes it's necessary to avoid rerenders but I'd probably prefer to start without trying to optimize everything.

Other than that, the changes are good.

const isRTL = useSelect( ( select ) => {
return select( 'core/block-editor' ).getSettings().isRTL;
} );
const toolbarControls = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

In the previous version, we were not using a memoized prop, do you think it's really necessary here? I always wonder if we're over-optimizing sometimes, My personal opinion would be to start with a non-optimized version unless we measure change in performance.

setTextColor,
textColor,
} ) {
const colorSettings = useMemo(
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

);
}

function ParagraphToolbar( { direction, setDirection } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be renamed ParagraphRTLToolbar instead?

@jorgefilipecosta jorgefilipecosta force-pushed the update/refactor-paragraph-block-to-be-a-functional-component branch from f453813 to 5fa6e6d Compare November 12, 2019 17:03
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

Looks great, thanks 👍

@jorgefilipecosta jorgefilipecosta merged commit 7d64e01 into master Nov 12, 2019
@youknowriad youknowriad added this to the Gutenberg 7.0 milestone Nov 25, 2019
@aduth aduth deleted the update/refactor-paragraph-block-to-be-a-functional-component branch December 6, 2019 00:32
@aduth
Copy link
Member

aduth commented Dec 6, 2019

In investigating typing performance regressions between Gutenberg 6.9 and Gutenberg 7.0, something as significant a refactor to the paragraph block like this seems like it could be a prime candidate as a contributor. It would have been good to do some performance measurements of this refactor (though generally I'd have hoped only for improvements).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants