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

Convert BlockHTML component from class component to React hooks #18968

Merged
merged 2 commits into from
Dec 6, 2019

Conversation

youknowriad
Copy link
Contributor

Just a small refactoring PR that shouldn't have any impact aside improving our code quality and following the last guidelines (prefer react hooks)

Testing instructions

Ensure that "Edit HTML" works as before.

@youknowriad youknowriad added the [Type] Code Quality Issues or PRs that relate to code quality label Dec 6, 2019
@youknowriad youknowriad self-assigned this Dec 6, 2019
};

useEffect( () => {
setHtml( getBlockContent( block ) );
Copy link
Member

Choose a reason for hiding this comment

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

I notice before that there was an isEqual check. Do you remember why? Does it make sense here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's not necessary because the useState hook won't rerender if we end up with the same html content.

Copy link
Member

Choose a reason for hiding this comment

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

But the useEffect hook will still be called? I guess it's ok to re-set the HTML?

@youknowriad
Copy link
Contributor Author

I removed the previous test because it was testing the implementation details and not really the behavior.

@youknowriad youknowriad merged commit 10596a5 into master Dec 6, 2019
@youknowriad youknowriad deleted the update/convert-block-html branch December 6, 2019 13:08
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants