-
Notifications
You must be signed in to change notification settings - Fork 377
chore(Dropdown): convert examples to TypeScript #7388
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-7388.surge.sh A11y report: https://patternfly-react-pr-7388-a11y.surge.sh |
nicolethoen
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 split button action and split button primary action examples are missing a gap between the dropdowns. It's a little confusing because it looks like a single dropdown toggle with a lot of icons/buttons on it.
61179c9 to
5d084d2
Compare
Ah good catch! I added back in the |
jenny-s51
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.
Demos LGTM, nice work Eric !
The checkbox guidelines you linked refer to the 3rd state as an "intermediate" state so we should try to match those!
Added some comments where these occurrences can be updated 🙂
packages/react-core/src/components/Dropdown/examples/DropdownSplitButtonIndeterminate.tsx
Show resolved
Hide resolved
packages/react-core/src/components/Dropdown/examples/DropdownSplitButtonIndeterminate.tsx
Show resolved
Hide resolved
packages/react-core/src/components/Dropdown/examples/DropdownSplitButtonIndeterminate.tsx
Show resolved
Hide resolved
packages/react-core/src/components/Dropdown/examples/DropdownSplitButtonIndeterminate.tsx
Show resolved
Hide resolved
|
@jenny-s51 I had asked Margot about that intermediate vs indeterminate verbiage, and she ended up making an update to the design guidelines for the checkbox to use "indeterminate". |
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.
Awesome! Yes, indeterminate definitely makes more sense here. In that case LGTM! 🚀
What: Closes #7383
Dropdown component
I also updated the example for for "Split button (3rd state)" to "Split button (indeterminate state)" since that seems a more appropriate/descriptive name (this state is also mentioned in the when to use switches versus checkboxes section of the Checkbox design guidelines, though the term "intermediate" is used instead).
Additional issues: