Skip to content

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

Merged
jorgefilipecosta merged 2 commits intomasterfrom
update/refactor-paragraph-block-to-be-a-functional-component
Nov 12, 2019
Merged

Update: Refactor paragraph block to be a functional component#18125
jorgefilipecosta merged 2 commits intomasterfrom
update/refactor-paragraph-block-to-be-a-functional-component

Conversation

@jorgefilipecosta
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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