feat(Button/MenuToggle): added support for hamburger/settings animations #11861
Conversation
thatblindgeye
changed the title
thatblindgeye
requested review from
nicolethoen,
andrew-ronaldson,
mcoker and
kmcfaul
|
Preview: https://patternfly-react-pr-11861.surge.sh A11y report: https://patternfly-react-pr-11861-a11y.surge.sh |
We could have a custom button section for the favorite, settings, hamburger and future wacky buttons.
I don't know that we'd force a full change but what if in the next breaking change the animated one is the default and the old version a variant? |
| @@ -27,7 +27,7 @@ export const ClipboardCopyToggle: React.FunctionComponent<ClipboardCopyTogglePro | |||
| onClick={onClick} | |||
| id={id} | |||
| aria-labelledby={`${id} ${textId}`} | |||
| aria-controls={contentId} | |||
| aria-controls={isExpanded ? contentId : undefined} | |||
There was a problem hiding this comment.
This update needs to be made as it resolves a critical axe error where the aria-controls attr references a non-existent ID (since the expandable content is dynamically rendered)
|
From 6/10/25 animations sync meeting: Remove logic that restricts variant and children, replace cog icon in "with icons" with plus icon, remove 4th hamburger button example |
|
At 1200px, the arrow version of the hamburger briefly flashes - is there a > vs. >= sort of thing going on that could be smoothed out? 2025-06-11_16-52-46.mp4 |
thatblindgeye
force-pushed
the
iss11370_SettingHamburgBtns
branch
from
27f3073 to
1ef2e90
Compare
|
Per discussion between @rebeccaalpert , @mcoker and myself yesterday, we decided to keep this custom hamburger icon in the codebase (just moved to the Button directory) since it's such a specific icon that requires being wrapped in a |
| // Because this is such a specific icon that requires being wrapped in a pf-v[current version]-c-button element, | ||
| // we don't want to export this to consumers nor include it in the react-icons package as a custom icon. | ||
| export const hamburgerIcon = ( | ||
| <svg viewBox="0 0 10 10" className="pf-v6-c-button--hamburger-icon pf-v6-svg" width="1em" height="1em"> |
There was a problem hiding this comment.
nit - can you import the react-tokens for these classes and use token.name to populate the classes instead of hardcoding them?
| import { MenuToggle, Flex } from '@patternfly/react-core'; | ||
|
|
||
| export const MenuToggleSettings: React.FunctionComponent = () => ( | ||
| <Flex> |
There was a problem hiding this comment.
nit - just an inconsistency, but I noticed the updated custom icon example puts {' '} between the items for a gap instead of wrapping them in <Flex />.
thatblindgeye
force-pushed
the
iss11370_SettingHamburgBtns
branch
from
7f8c60a to
f4f6553
Compare
thatblindgeye
force-pushed
the
iss11370_SettingHamburgBtns
branch
from
f4f6553 to
e106295
Compare
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #11370
Commit 1 adds component logic and new examples.
Commit 2 updates existing examples/demos to use these new implementations.
Commit 6 adds new tests for these new features
Commit 7 makes updates from 6/10/25 animations sync meeting to not restrict the variant nor children for either type of new button (can compare to Commit 1 and 6)
Commit 8 cleans up some examples and prop verbiage
Questiosn:
isSettingsjust applies the necessary modifier class.Assumptions I had made in the code that can use design input:
variantfor either of these types of buttons.iconprop (so a consumer is stuck with the CogIcon we ship a "settings" MenuToggle with, but it can be a plain, primary, secondary, etc MenuToggle).Additional issues: