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

Navigation: Add clearable option to color picker in navigation block #68454

Merged
merged 1 commit into from
Jan 10, 2025

Conversation

yogeshbhutkar
Copy link
Contributor

@yogeshbhutkar yogeshbhutkar commented Jan 2, 2025

What, Why and How?

This PR introduces a clearable button, allowing users to easily clear applied colors. It ensures consistency between the Color Picker in the Sub Menu and the one in the Navigation block.

The clearable option has been added to the settings for all applicable settings, effectively addressing the issue.

Testing Instructions

  1. Add a Navigation block to your page.
  2. Apply a Background Color to the block.
  3. Use the Clear button to remove the applied color and verify that it works as expected.

Screenshots

Before After
Screenshot 2025-01-02 at 11 38 39 AM Screenshot 2025-01-02 at 11 29 39 AM

Screencast

Screencast

Closes: #68453

@yogeshbhutkar yogeshbhutkar changed the title Navigation: Add clearable option for color tools in navigation edit Navigation: Add clearable option to color picker in navigation block Jan 2, 2025
@yogeshbhutkar yogeshbhutkar marked this pull request as ready for review January 2, 2025 06:42
Copy link

github-actions bot commented Jan 2, 2025

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: yogeshbhutkar <[email protected]>
Co-authored-by: Mayank-Tripathi32 <[email protected]>
Co-authored-by: hbhalodia <[email protected]>
Co-authored-by: t-hamano <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

Copy link
Contributor

@Mayank-Tripathi32 Mayank-Tripathi32 left a comment

Choose a reason for hiding this comment

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

LGTM, Works as expected.

Copy link
Contributor

@hbhalodia hbhalodia left a comment

Choose a reason for hiding this comment

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

Not sure if the failing tests are related, but it should resolve. Approving for code logic.

@yogeshbhutkar
Copy link
Contributor Author

Hi @t-hamano, can you please review this PR when you get a moment?

@t-hamano
Copy link
Contributor

t-hamano commented Jan 7, 2025

I think we should first consider whether we need a consistent clear button. Because there is precedent in the past where the need for this clear button has been debated and removed (#42689).

@WordPress/gutenberg-design Is this clear button necessary in the current Gutenberg?

By the way, currently there is a reset button that appears when you hover the mouse over an item if it has a color value. Whether this reset button is displayed or not is synchronized with the clearable prop, that is, the Clear button in the popup:

image

@t-hamano t-hamano added [Type] Enhancement A suggestion for improvement. Needs Design Feedback Needs general design feedback. [Block] Navigation Affects the Navigation Block labels Jan 7, 2025
@yogeshbhutkar
Copy link
Contributor Author

Thanks for the review @t-hamano.

there is a reset button that appears when you hover the mouse over an item if it has a color value.

This feature becomes quite helpful when clearing the colors.

The Heading and Rich Text components already include a clear button, as do many other blocks. However, it seems the Navigation block is missing this feature, which I believe creates an inconsistency.

Screenshot 2025-01-07 at 5 56 38 PM

@t-hamano t-hamano self-requested a review January 10, 2025 13:17
Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

I noticed that PRs similar to this one were merged recently:

Maybe it's better to have a consistent clear button now, so I'm accepting this PR. Sorry for the confusion 🙇‍♂️

However, if the button text is long, the reset button will cover the text:

image

We'll need to address this issue separately.

@t-hamano t-hamano merged commit 22fbc32 into WordPress:trunk Jan 10, 2025
74 of 77 checks passed
@github-actions github-actions bot added this to the Gutenberg 20.1 milestone Jan 10, 2025
@yogeshbhutkar
Copy link
Contributor Author

We'll need to address this issue separately.

I've taken this case into account and am working on it. Thanks for the review!

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

Successfully merging this pull request may close these issues.

Inconsistency in Clear Color Option in Navigation block.
4 participants