[WIP] Colors: Fix color button border radii #55071
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronrobertshaw
added
[Type] Bug
|
@ciampo or @mirka do you have any thoughts on this approach? The catch is the reliance on The alternative is needing an additional prop and further complexity in the ToolsPanel components. The first/last classes were only added to support the item group styling the color panel needs however as non item group controls are being added to the the color panel, the end styling is a bit off. If making an exception here so the color panel styles do rely on internal component class/name, we could simplify and clean up the component removing some of the experimental props in the process. Would that be worth the trade off? |
|
Size Change: +100 B (0%) Total Size: 1.64 MB
ℹ️ View Unchanged
|
|
Left a reply in the reported issue |
|
Thanks @ciampo, I should have foreseen the questions about Item and ItemGroup coming up again 🤦 I've updated the PR's description for anyone else that might join the discussion via this PR. |
|
Closing in favour of #55207 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes: #55051
What?
Applies the intended border radii to the last color item regardless of whether additional controls have been injected into the colors panel.
Why?
The details matter.
Why not
ItemandItemGroup?This was explored when the color panel was initially switched to the
ToolsPaneland ad hoc controls were allowed to be rendered into it via a slot. See #34027.TL;DR
firstandlastclasses were the accepted workaround to allow pseudo ItemGroup stylingHow?
Leverages
nth-child(1 of selector)andnth-last-child(1 of selector)to target the first and last items of a given class that haven't been hidden via the placeholder item style.These selectors are working for me in: Chrome, Safari, Firebox, Edge, and Opera.
NOTE: One downside here is that we rely on a partial class match for the final generated class name for hidden placeholder items.
If this approach was accepted we could simplify and clean up the ToolsPanel components to remove the need for first/last classes to be applied.
An alternative approach here is that we could pass an additional prop to the ToolsPanel allowing for the first/last item calculation to filter on a class name as well. This would avoid depending on a component class but does alter the component's API and complicate the code further.
I've gone with CSS only as the simplest option first but am happy to go the alternative route if so desired.
Testing Instructions
Testing Instructions for Keyboard
Changes are visual only.
Screenshots or screencast