Global Styles Sidebar: Clean up button spacing and semantics #40533
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
mirka
added
Needs Design Feedback
|
@jasmussen I'd love to get your design feedback here 🙏 Also if you or @critterverse already have a decision on #40478, we could probably work that into this PR stack too! |
|
Size Change: +24 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
|
Nice one, seems like a good improvement overall. Trunk looks like this:
This PR makes it look much more considered in the spacing between buttons and separators: One small thing — the descriptive text for "Blocks" lines up better in trunk. There, it shares left margin with the button: In this PR the description goes closer to the edge: Looking back to #38934, it seems this PR brings us very close, and mainly that paragraph that stands out. Nice work! |
|
Thanks for the quick review @jasmussen!
Highlighting this note in my OP in case it was skimmed over:
With that in mind, should we just go with an ad hoc approach here? I'm hesitant to turn this into a legitimate pattern unless we have a clear design guideline (e.g. inset all adjacent descriptions and headings when an ItemGroup is borderless). To be scalable, I think it should be visually and semantically clear which node the description is associated with — is it describing the entire
|
|
Sorry I missed that. I think we should be ad-hoc here, but leave a comment in the CSS calling out the incongruety. That way, when we come in to revisit, we'll know that it's safe to remove. In general I share the desire to avoid hacks and bespoke CSS like this, it's not good form. In this case, however, we know that it will be revisited, and in the not too distant future as well. #40478 might remove the descriptive text entirely, and the separate explorations in #36667 (comment) recast Global Styles in a dark context where the paddings will need a new approach regardless. What do you think? |
|
Yep, that sounds perfectly reasonable to me 👍 Thanks for the thoughtful replies, as always. I'll whip up something to recreate the inset. |
mirka
added
[Status] In Progress
This is temporary. Will fully address in a separate PR.
This was referenced Apr 25, 2022
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.
Part of #38934
Stacked on #40526
🚧 Status: Requesting design feedback (Later I'll be splitting this PR for easier code review)
What?
Cleans up spacing and semantic issues with navigator buttons in the Global Styles Sidebar.
Why?
To refine the visual consistency of the Global Styles sidebar.
How?
NavigationButtonabstraction toNavigationButtonAsItemso it's clearer that it's anItem, and thus needsItemGroupparent.Testing Instructions
npm run devScreenshots or screencast
Main screen
Updated to reflect final tweaks.
Header
More after shots