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

Refactor Latest Comments block to use function component #23557

Merged
merged 2 commits into from
Oct 14, 2020

Conversation

bartczyz
Copy link
Contributor

Description

Related to #22890.

Types of changes

Refactor LatestComments to use function component.

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.

@ZebulanStanphill ZebulanStanphill added [Block] Latest Comments Affects the Latest Comments Block [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality labels Jun 29, 2020
<ToggleControl
label={ __( 'Display avatar' ) }
checked={ displayAvatar }
onChange={ () =>
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you could keep the generic toggle function, since all three toggle on change like:

const toggleAttribute = ( propName ) => () =>
	setAttributes( { [ propName ]: ! attributes[ propName ] } );

Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced it would make much of a difference, performance-wise.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's more about reusability and not performance.

displayAvatar,
displayDate,
displayExcerpt,
} = attributes;
Copy link
Contributor

Choose a reason for hiding this comment

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

You can destructure attributes in the above destructuring.

{ attributes, attributes: { commentsToshow, ....... } setAttributes }

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it would be { attributes: { commentsToShow, (etc) }, setAttributes }. No need to duplicate attributes; actually, I think that would cause a linting error.

Copy link
Member

@ZebulanStanphill ZebulanStanphill left a comment

Choose a reason for hiding this comment

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

This looks good to merge. It just needs a rebase to get tests passing.

@youknowriad youknowriad merged commit 9406c12 into WordPress:master Oct 14, 2020
@github-actions github-actions bot added this to the Gutenberg 9.2 milestone Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Latest Comments Affects the Latest Comments Block [Package] Block library /packages/block-library [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants