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

Social Icons: Fix color picker bug when set to Logos Only #38006

Merged

Conversation

ndiego
Copy link
Member

@ndiego ndiego commented Jan 15, 2022

Description

When the Social Icons block is set to "Logos Only", the color picker for the Icon Background color should not be shown. Currently it is still shown but with no label and no settings. Basically just an "empty" color picker component is rendered. This PR fixes that by removing the conditional from the color settings array itself.

How has this been tested?

  1. Use the latest version of Gutenberg and the latest version of TT2
  2. Add a Social Icons block to a page and add a few Social Links.
  3. Set the Social Icons block to "Logos only", see the "empty" color picker component.
  4. Switch to this PR and now the color picker displays as it should.

Screenshots

Currently in trunk:
image

With this PR:
image

Types of changes

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 (please manually search all *.native.js files for terms that need renaming or removal).
  • I've updated related schemas if appropriate.

@ndiego ndiego added [Type] Bug An existing feature does not function as intended [Block] Social Affects the Social Block - used to display Social Media accounts labels Jan 15, 2022
@ndiego ndiego requested a review from mkaz as a code owner January 15, 2022 14:46
@ndiego ndiego self-assigned this Jan 15, 2022
@ndiego ndiego requested a review from Mamaduka January 15, 2022 15:10
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @ndiego 👍

It appears this bug was introduced via the PR (#37067) switching the PanelColorGradientSettings to use dropdowns. Previously, the ColorGradientControl would return null when the settings object was missing anything that was required. Wrapping that component within the dropdown without also moving or duplicating those necessary checks gives the empty control in the colors panel.

I wonder if it's possible there might be more cases of this out in the wild. @jorgefilipecosta do you think it is also worth adding a check to the color dropdown generation to guard against this issue?

As for this PR specifically, it does solve the problem for this block.

✅ I could replicate the issue on trunk
✅ After applying these changes the empty control is no longer shown for the logos only block style.

Before After
Screen Shot 2022-01-17 at 11 34 17 am Screen Shot 2022-01-17 at 11 35 22 am

@ndiego
Copy link
Member Author

ndiego commented Jan 17, 2022

@aaronrobertshaw thanks for the approval. So do you think we should merge this or would adding the check to the color picker component be a more appropriate fix?

@jorgefilipecosta jorgefilipecosta merged commit ee9c159 into WordPress:trunk Jan 17, 2022
@github-actions github-actions bot added this to the Gutenberg 12.5 milestone Jan 17, 2022
@jorgefilipecosta
Copy link
Member

Hi @ndiego, thank you for submitting this change I think the change is good regardless of a more general fix. I will look into a more general fix just in case we have other usages we missed.

@ndiego ndiego deleted the fix/color-picker-in-social-links-block branch January 17, 2022 15:34
@aaronrobertshaw
Copy link
Contributor

So do you think we should merge this or would adding the check to the color picker component be a more appropriate fix?

@ndiego I was planning to merge this today pending Jorge having a chance to take a look. I see he's already merged which is great. Thanks again for the fix.

@ndiego
Copy link
Member Author

ndiego commented Jan 18, 2022

Thanks @aaronrobertshaw!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Social Affects the Social Block - used to display Social Media accounts [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants