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

Add check for button text before rendering button block #29717

Merged
merged 1 commit into from
Mar 10, 2021

Conversation

getsource
Copy link
Member

Description

This PR continues the code and conversation from #24064, so props @richtabor, @ZebulanStanphill, and @youknowriad from there.

@Mamaduka also helped put this PR together and with testing.

This PR adds a check for button text before rendering the Button block on the front-end. This prevents an empty <div class="wp-block-button"><a class="wp-block-button__link"></a></div> from displaying on the front-end.

Closes #17221.

How has this been tested?

Tested using wp-env and trunk.
Locally, with wp-env, tests pass as well.

Screenshots

Before:

Editor:
Editor - Before
Front-end:
Front-end Before

After:

Editor:
Editor - After

Front-end:
Front-end - After

Types of changes

Bug fix.
It does, however, change how buttons without text are displayed after a post with such buttons is loaded, then saved again.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • 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.

@getsource getsource changed the title Add check for text before rendering button block Add check for button text before rendering button block Mar 10, 2021
@youknowriad youknowriad requested a review from a team March 10, 2021 14:45
@youknowriad youknowriad added [Block] Buttons Affects the Buttons Block [Type] Enhancement A suggestion for improvement. labels Mar 10, 2021
Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Tested this and worked like a charm. The code looks good as well 🚢

@draganescu draganescu merged commit 2304e70 into WordPress:trunk Mar 10, 2021
@github-actions github-actions bot added this to the Gutenberg 10.2 milestone Mar 10, 2021
rel={ rel }
/>
</div>
text && (
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: you could have sed and early return here. Not that it changes anything, just wanted to share this code style trick in case you find it nicer.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you!
I considered that when I saw the previous PR, but when I tried it, I got a lot of warnings/errors regarding unused variables (from everything included in the return that was skipped).

Is there a way to work around that, for future knowledge at least? I think this is pretty readable, but an early return is probably slightly better.

Copy link
Member

Choose a reason for hiding this comment

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

The answer is to just move the early return above the unused variables. I went ahead and made a PR for this: #29781.

Copy link
Contributor

@youknowriad youknowriad Mar 11, 2021

Choose a reason for hiding this comment

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

That PR is good. Though I would like to add just a small thing here. Early returns in components can break the "react hooks rules", so if we use this technique, we should make sure there's no React hook being called after the early return. (it's not the case here)

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad Good point. That kind of thing is caught by the React rules-of-hooks ESLint rules, isn't it? Or are we not using that?

Copy link
Contributor

Choose a reason for hiding this comment

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

you're probably right :)

Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad that rule doesn't apply to custom hooks like useBlockProps?

Just want to know for future reference.

Copy link
Member

Choose a reason for hiding this comment

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

@Mamaduka useBlockProps.save() is not a hook. In fact, React hooks aren't even usable in a block's save implementation. It's actually just an alias that points to getBlockProps() from https://github.com/WordPress/gutenberg/blob/trunk/packages/blocks/src/api/serializer.js. Yeah, it's kinda weird.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, @ZebulanStanphill for clarifying this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Button Block: Only display buttons if the title attribute exists
5 participants