feat(menu-toggle): added tokens, new design spec#6177
Conversation
|
Preview: https://patternfly-pr-6177.surge.sh A11y report: https://patternfly-pr-6177-a11y.surge.sh |
fb990c9 to
1ac39d6
Compare
917ce92 to
04ca11c
Compare
2adbc92 to
c4be0d6
Compare
84f097c to
bffc98d
Compare
|
Looks like this split expanded state is using |
bffc98d to
bcef1b6
Compare
bcef1b6 to
97c03cd
Compare
|
In this example, https://patternfly-pr-6177.surge.sh/components/menus/menu-toggle/#plain should the 3rd one show as expanded? And should the 5th one be marked as disabled? |
There was a problem hiding this comment.
Thanks Matt!! looking great. I noticed a few things --
-
Hover and expanded states for the regular toggles should be using
border—color—hoverandborder—color—clickedrespectively, not brand--hover/clicked. -
Plain with text toggle looks like it needs the mix blend mode token so the hover shows on dark theme.
-
Split action has a blue divider in between on the default state, that should probably just point to border--color--default.
-
Split action and split secondary only have one hover state in the PR, but the action and dropdown toggle should have separate hover/clicked interactions for both of those.
Otherwise looks good! thank you!
There was a problem hiding this comment.
- @lboehling For the split button toggles, should each variant have separate hover styling for the action and the toggle button? The primary split button action for example only changes the style on hover for the button you're hovering. The default and secondary don't (at least I couldn't really tell)
- For the typeahead, the outline for the arrow toggle button gets a little cutoff/overlapped by I assume the text input group border (screenshot below). This is more a nit because I barely noticed it, but I know we've noticed similar issues in the past where the outline gets cutoff so just wanted to note it
- This can be a followup, but it'd be nice to update examples so that we state what toggle is in what state. The primary and secondary examples do this, with the toggle text being "Collapsed" etc, but the plain toggle, split button and such examples aren't as clear unless you dig through the code.
@thatblindgeye I had the same question. I believe for the Alpha release, we're going to stick with the current interaction
@thatblindgeye There are several other changes required for this. In the interest of the Alpha release, I created this followup to address text input group followup 6257
@thatblindgeye I also left many |
There was a problem hiding this comment.
Above sounds good. Few more things I noticed which if they wouldn't be covered by the todo's you've noted, we can add on as a followup. Don't think anything stands out as blocking to me otherwise so I'd be good giving it the ✅
- The split button primary: it may not be immediately apparent that there is a checkbox that can be clicked when it's checked:
| {{> menu-toggle menu-toggle--UsesButton=true menu-toggle--IsPlain=true menu-toggle--IsDiv=true}} | ||
| | ||
| {{#> menu-toggle menu-toggle--IsPlain=true menu-toggle--attribute='aria-label="Actions"'}} |
There was a problem hiding this comment.
Do we plan to support both a div wrapper and button wrapper for instances like this? With the div wrapper the focus styling ends up not working correctly, since those styles end up on the div.pf-ct-c-menu-toggle. There's still an outline, but the background isn't changing like I'd expect (at least in dark mode).
There was a problem hiding this comment.
Not both, transitionally we need to support button.pf-v5-c-menu-toggle__button, but should be using .pf-v5-c-button.
| --#{$menu-toggle}--active--BackgroundColor: var(--pf-t--global--background--color--control--default); | ||
| --#{$menu-toggle}--active--BorderWidth: var(--pf-t--global--border--width--button--active); | ||
| --#{$menu-toggle}--active--BorderColor: var(--pf-t--global--border--color--clicked); |
There was a problem hiding this comment.
@lboehling is there a use case for active/clicked styling of a menu toggle that would differ from its expanded state?
3804795 to
3f1824e
Compare
There was a problem hiding this comment.
My comments above can be addressed as a followup, can we add those comments to the issue you created to track followups needed? Otherwise lgtm
There was a problem hiding this comment.
Good stuff! Thanks
3f1824e to
13da16f
Compare
|
🎉 This PR is included in version 6.0.0-alpha.83 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #6163
Menu toggle: https://patternfly-pr-6177.surge.sh/components/menus/menu-toggle
Text input group will be addressed in followup 6257