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

Incorrect border radius when only one color panel item #55051

Closed
richtabor opened this issue Oct 4, 2023 · 6 comments · Fixed by #55207
Closed

Incorrect border radius when only one color panel item #55051

richtabor opened this issue Oct 4, 2023 · 6 comments · Fixed by #55207
Assignees
Labels
[Feature] Colors Color management [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@richtabor
Copy link
Member

richtabor commented Oct 4, 2023

The border radius is incorrect if there is only one color panel item. It should be $radius-block-ui on all corners.

We currently add .first and .last classes to the .block-editor-tools-panel-color-gradient-settings__item, but they don't account for when there is only one item.

You can see this example within the Featured Image block.

Current Expected
CleanShot 2023-10-04 at 12 18 28 CleanShot 2023-10-04 at 12 19 19
@richtabor richtabor added [Type] Bug An existing feature does not function as intended [Feature] Colors Color management labels Oct 4, 2023
@aaronrobertshaw aaronrobertshaw self-assigned this Oct 5, 2023
@aaronrobertshaw
Copy link
Contributor

We currently add .first and .last classes to the .block-editor-tools-panel-color-gradient-settings__item, but they don't account for when there is only one item.

To clarify, the .first and .last classes are added to the ToolsPanelItems rendered into the ToolsPanel rather than the .block-editor-tools-panel-color-gradient-settings__item elements.

The catch here is that the color panel actually has two tools panel items. The second one is the overlay slider that was injected into the color panel in #43838. It receives the .last class.

Screenshot 2023-10-05 at 1 05 23 pm

I'll see what I can do to get the correct border radii on the color items.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Oct 5, 2023
@aaronrobertshaw
Copy link
Contributor

One option in terms of a fix is available in #55071 however it relies on a generated class name by the ToolsPanel components so might not be acceptable.

@ciampo
Copy link
Contributor

ciampo commented Oct 5, 2023

Hey folks, I took a look and I'll leave a few thoughts:

  • Is there a reason why we're not using Item and ItemGroup ? Those components should already automatically embed functionality for displaying border and having rounded corners only on the first/last item (example)
  • Alternatively, if we want to continue to rely on a Button + custom styles, what if we changed the logic of the CSS to be able to target first and last instances of those buttons without relying on .first and .last classnames — I quickly put together an example of how we could about it here . The only caveat is that it relies on :has, which is not supported by Firefox yet (although it should be in the upcoming weeks, although I don't see that as a blocker.

@aaronrobertshaw
Copy link
Contributor

Thanks for weighing in @ciampo 👍

Is there a reason why we're not using Item and ItemGroup

This was explored when the color panel was converted to the tools panel.

Off the top of my head, there were a couple of issues with this including the fact that non-Item controls could be rendered into the panel's slot and that to ensure consistent order of controls those that aren't currently being displayed still require a placeholder element to hold the position in the DOM.

@aaronrobertshaw
Copy link
Contributor

#34027 provides some context as to why Item and ItemGroup couldn't be used.

Alternatively, if we want to continue to rely on a Button + custom styles, what if we changed the logic of the CSS to be able to target first and last instances of those buttons without relying on .first and .last classnames

I don't think this covers the situation where hidden placeholder elements are in the DOM to maintain order of controls given slots don't guarantee the order of their fills. In the example sandbox linked if you add display: none to the last item you'll see the border radii disappear.

The first/last classes were added to avoid the need to target the dynamic classes generated by the component.

@ciampo
Copy link
Contributor

ciampo commented Oct 10, 2023

Trying an alternative approach in #55207, hopefully it works well!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
Status: Done
3 participants