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

Format Library: Prevent the text and color picker from overlapping #69169

Merged

Conversation

shimotmk
Copy link
Contributor

What?

Ensure that inline formatting does not cover the text it is set to

Why?

When the inline format color palette is displayed, it may overlap with the component-color-palette__custom-color-button, the button that indicates the currently selected color. By adding __experimentalIsRenderedInSidebar, it will not overlap with this button, and you can adjust the color of the text you are setting while checking it!

The sidebar's global style already has __experimentalIsRenderedInSidebar set, so the buttons and color picker don't overlap.

How?

Testing Instructions

  1. Open a post or page. -->
  2. Insert a Paragraph block.
  3. When you set an inline text color, you'll see your custom color appear next to the popover.

Testing Instructions for Keyboard

Screenshots or screencast

After

after-inline.mp4

Before

before-inline-color.mp4

label:"[Package] Format library" "[Feature] Colors"

@shimotmk shimotmk requested a review from ellatrix as a code owner February 12, 2025 22:30
Copy link

github-actions bot commented Feb 12, 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: shimotmk <[email protected]>
Co-authored-by: Mamaduka <[email protected]>

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

@Mamaduka Mamaduka added [Type] Bug An existing feature does not function as intended [Package] Format library /packages/format-library [Feature] Colors Color management labels Feb 13, 2025
@shimotmk
Copy link
Contributor Author

@Mamaduka
Could you see this change?

@Mamaduka
Copy link
Member

Sorry for the delay, @shimotmk!

Do you mind rebasing on top of the latest trunk? I would like to do one final test, and then I think this is good to merge.

@shimotmk
Copy link
Contributor Author

@Mamaduka Thank you. I rebased it.

Copy link
Member

@Mamaduka Mamaduka left a comment

Choose a reason for hiding this comment

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

Thank you, @shimotmk!

The fix works as expected ✅

@Mamaduka Mamaduka changed the title Format-library: Text-color ColorPalette IsRenderedInSidebar Format Library: Prevent the text and color picker from overlapping Feb 25, 2025
@Mamaduka Mamaduka enabled auto-merge (squash) February 25, 2025 05:37
@Mamaduka Mamaduka merged commit e532b74 into WordPress:trunk Feb 25, 2025
60 checks passed
@github-actions github-actions bot added this to the Gutenberg 20.4 milestone Feb 25, 2025
@shimotmk shimotmk deleted the update/text-color/renderd-in-sidebar branch February 25, 2025 23:22
@ciampo
Copy link
Contributor

ciampo commented Feb 27, 2025

The __experimentalIsRenderedInSidebar prop is (as the name implies) used when ColorPalette is rendered in the sidebar. Using it outside of its scope can lead to unexpected behaviour and breakage in the future, could you undo this change?

@Mamaduka
Copy link
Member

Using it outside of its scope can lead to unexpected behaviour and breakage in the future, could you undo this change?

I thought it was okay with a note, but I don't have a strong opinion here.

@ciampo
Copy link
Contributor

ciampo commented Feb 27, 2025

Up to you, ultimately. The __experimentalIsRenderedInSidebar is already a bad abstraction that I would have liked to remove (it was added sneakily) in favor of allowing for positioning the popover in a more decoupled way.

You can keep it around if really needed, it will just add a little bit of extra tech debt.

@mirka
Copy link
Member

mirka commented Feb 27, 2025

Right, I guess it's fine as a quick bandaid fix, but the real TODO item here is to fix the API 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management [Package] Format library /packages/format-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants