Navigation: Experiment with Divider component for setup state #36302
Conversation
|
I wanted to just emphasize again that this PR is a draft. The metrics in the placeholder are wrong in a few ways:
Those are all things I hope to fix up and polish. But instead of writing bespoke CSS for the navigation block, I'd love to start leveraging these components so we fix things in a more scalable way. |
| borderColor: 'black', | ||
| orientation: 'vertical', | ||
| }; | ||
| const dividerStyles = { |
There was a problem hiding this comment.
If I set the orientation to vertical, it seems like the component could be smart enough that the only styles I needed to define were height (if I wanted anything other than auto), and possibly left/right margins. It seems like we could provide nice defaults for margins and colors so those would be accurate without overriding in most cases.
jasmussen
force-pushed
the
polish/navigation-setup-state
branch
from
48d0e39
to
de27441
Compare
|
Size Change: -89 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
|
Just for anyone subscribed to this one, I now expect to not land this one, but rather port some of the fixes to a separate PR and take a different approach. I still think the questions posed around the Divider component are worth thinking about, but it's obviously not urgent. |
|
Closing this one in favor of #36302, though I'd still appreciate feedback on the thoughts shared! |
There was a problem hiding this comment.
Hey @jasmussen !
Just leaving some comments here (even if this PR was closed)
Am I setting the orientation correctly?
Yes! The Divider components inherits from Reakit's Separator, and you're setting theorientation prop correctly :) (cc @diegohaz)
By default the intrinsic styles appear optimized towards a horizontal separator. If we mean to embrace the vertical orientation as a property, should the component itself come with some styles that work better for this orientation?
What do you have in mind for "horizontal orientation" styles?
border might be the right property to set for a divider (as opposed to for example a background color paired with a border-width), but it does mean that the margins are off. For a 12px right margin I have to apply 11px because I need to subtract 1px of border. Can that be simplified?
I assume that you're referring to using the margin* props on the Divider, and how the "border width" of the Divider line adds 1px to the overall space occupied by the component.
I'm not sure what's the best solution here. On one hand, I understand your point of view. On the other hand, as a developer who is used to work with CSS, the way the component currently work is not confusing to me, as in my mental model I treat the Divider's size separately from its margins.
This might be intrinsic to these components, but the styles applied are replicated across ever instance of the component. Outside of making the component itself smarter so it can absorb some of the styles I had to write, is this expected?
It is expected that each instance of Divider (or any component) will render separately from other instances — i.e. won't "absorb" auto-magically styles set on other Dividers.
There are definitely ways you could cope with this when importing Divider — for example, wrapping Divider into a custom component which sets some custom props and styles.
Another potential way to solve this is to leverage some sort of "theming" — for example, you could write a custom theme for the Divider component, and apply it to all Dividers in a certain portion of a React app. Unfortunately, this is not implemented yet in @wordpress/components. We've recently started talking about it (Sara even opened a proof of concept PR here, but we can't guarantee that theming support is going to land soon.
I'd love to be able to use some variables for colors and margins, both inside and outside the component. What's the best way to do this? For example we should virtually never use pureblack in any of the components, for UI the darkest color we use is $gray-900: #1e1e1e;.
The current state of our color configuration is a bit messy, as we're slowly moving away from SCSS/CSS and opting more and more components into using Emotion for styling.
Emotion-based components, in particular, use some shared configuration (defined in packages/components/src/utils/config-values.js). For example, the color of the Divider is set in that file. CC'ing @griffbrad here because I know that this topic has also been recently brought up somewhere else
If I set the orientation to vertical, it seems like the component could be smart enough that the only styles I needed to define were height (if I wanted anything other than auto), and possibly left/right margins. It seems like we could provide nice defaults for margins and colors so those would be accurate without overriding in most cases.
I'd need to take a deeper look at the component, but it does look like Divider may be optimized for the vertical case (and less for the horizontal one).
| const dividerProps = { | ||
| orientation: 'vertical', | ||
| }; | ||
| const dividerStyles = { | ||
| borderColor: 'black', | ||
| borderWidth: '0 1px 0 0', | ||
| borderStyle: 'solid', | ||
| width: '1px', | ||
| height: '18px', | ||
| marginTop: 'auto', | ||
| marginBottom: 'auto', | ||
| marginRight: '11px', | ||
| }; |
There was a problem hiding this comment.
Just as a FYI, it's usually better to avoid inline styles, and use classes instead!
Having said that, another FYI is that style can be passed directly as a prop:
const dividerStyles={ ... };
const dividerProps = {
orientation: 'vertical',
style: dividerStyles,
};
|
Thanks so much for taking the time to jot down these thoughts, it's definitely helpful for when I dive deeper into this stuff.
From what you are suggesting, the way I made the divider "vertical" was correct enough, and it did output the correct aria attribute. But what I found was that the visual appearance of the divider didn't change, it still appeared as a horizontal divider. And so my question was more in terms of what I, as a consumer of the component, should be supplying long term. There's a technical tinge to the question as well. How should those styles exist? Pseudo code: It's not something to solve urgently, but just figured it worth thinking about if this is a component we mean to start using. |
|
You definitely make a good point. Currently, I can see that We could look into adding vertical-friendly styles too, as you suggested! Not sure how much capacity I have currently to work on this, but it's definitely something we should work on. |
As far as I'm aware, the component isn't used at all in the codebase yet, so this was more of an observation than an urgent need. I imagine the issue could come up as part of a refactor of the block toolbar (woefully overdue), we can always look at it there. |
Description
This PR is mostly experimental as it uses the experimental
Dividercomponent in a way that I'm not completely sure about yet, so this is part learning how to use the components, part a potential to refine how they work. I'll get back to this in a moment.The intent of this PR is to polish the setup state to move closer to this mockup:
That is — with simpler buttons, clearer separation, and a smaller overall footprint.
Here's what the setup state looks like in trunk:
Here's this PR:
To get this PR home, though, I have some questions about how I'm using the
Dividercomponent:aria-orientationon thehrelement.bordermight be the right property to set for a divider (as opposed to for example a background color paired with a border-width), but it does mean that the margins are off. For a 12px right margin I have to apply 11px because I need to subtract 1px of border. Can that be simplified?blackin any of the components, for UI the darkest color we use is$gray-900: #1e1e1e;.Thanks for helping me understand these components! I'd be excited to help improve the Divider component itself!
How has this been tested?
Insert a Navigation block and select it.
Checklist:
*.native.jsfiles for terms that need renaming or removal).