-
Notifications
You must be signed in to change notification settings - Fork 375
Adds content to menu toggle component examples. #8500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Preview: https://patternfly-react-pr-8500.surge.sh A11y report: https://patternfly-react-pr-8500-a11y.surge.sh |
packages/react-core/src/components/MenuToggle/examples/MenuToggle.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/MenuToggle/examples/MenuToggle.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/MenuToggle/examples/MenuToggle.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/MenuToggle/examples/MenuToggle.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/MenuToggle/examples/MenuToggle.md
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/MenuToggle/examples/MenuToggle.md
Outdated
Show resolved
Hide resolved
2adca50 to
c7a6729
Compare
|
@edonehoo i'm noticing that the split menu toggle with secondary styling has been removed from the docs. was that a deliberate choice? |
|
So I was proposing the idea of removing it, but let me know what you think. My reasoning was that maybe the "Variant styles" example (formerly primary toggles) demonstrated the variant options that can be used, but if we want to be more explicit about secondary split button toggles (and/or secondary toggles, which I also removed) then I can add it back in. I like simplifying, but don't want to over-simplify! |
|
Would it be helpful/clear enough to add a row of toggles with secondary style below the row of toggles with primary style - all in the same example? |
|
I think that approach would work! Or an "isSecondary" toggle checkbox? I could add to the example content to point out both primary and secondary variants are shown in the example - if we wanted to be sure it's clear. |
fb45c7e to
e204fb5
Compare
|
@edonehoo I made a few updates to rearrange the example code a little. WDYT? |
thatblindgeye
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The examples are looking good!
Regarding the convo about showing the different variants with split button examples, I think either approach could work (showing split button with the variants vs just mentioning that in the description). Since currently the split examples show the primary and secondary variant, we should probably also update the "Split button toggle with checkbox" example to match as well (instead of showing the disabled/collapsed/expanded states). The "Variant styling can be applied to split button toggles..." verbiage could then be moved to that example instead of the "Split button with text label" example.
mcarrano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thanks @edonehoo !
| import { MenuToggle, TextInputGroup, TextInputGroupMain, TextInputGroupUtilities, Button } from '@patternfly/react-core'; | ||
| import TimesIcon from '@patternfly/react-icons/dist/esm/icons/times-icon'; | ||
|
|
||
| export const Typeahead: React.FunctionComponent = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we rename this to MenuToggleTypeahead for consistency.
7ae5d07 to
b46a343
Compare
tlabaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work Erin! LGTM
Makes progress on patternfly/patternfly-org#2990