-
Notifications
You must be signed in to change notification settings - Fork 368
feat(CC-batch-1): added batch 1 #11827
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
base: main
Are you sure you want to change the base?
Conversation
Preview: https://patternfly-react-pr-11827.surge.sh A11y report: https://patternfly-react-pr-11827-a11y.surge.sh |
cd2fe95
to
dbf3c61
Compare
dbf3c61
to
9f6488f
Compare
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.
Would each component benefit from an added comment linking directly to the component docs on patternfly.org? I think that'd be helpful.
}, | ||
example: (props) => ( | ||
<AccordionItem isExpanded={props.isExpanded}> | ||
<AccordionToggle>{props.toggleTextExpanded}</AccordionToggle> |
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 AccordionToggle should have an id="<your-id-here>" or something like that.
id` is a required prop.
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.
I'd also add a comment here recommending they implement an onClick
event to toggle the expanded state of this Accordion.
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.
+1
// enum | ||
isActive: figma.enum('State', { Active: true }), | ||
isReadOnly: figma.enum('State', { 'Read only': true }), | ||
isExpanded: figma.enum('State', { Expanded: true }), |
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.
is there a way in figma to make an expandable clipboardCopy variant? If so, then isExpanded
can be passed, but also the variant
needs to be 'expansion'
ActionList, | ||
'https://www.figma.com/design/aEBBvq0J3EPXxHvv6WgDx9/PatternFly-6--Components-Test?node-id=6780-15839&m=dev', | ||
{ | ||
props: { |
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.
Not sure if it's worth adding the isIconList
prop here or if there is no distinction in figma. In React this boolean prop adds a class for icon-only lists that removes some additional padding.
Relates to: #11624
Included components:
Component tracker
Figma preview
Resources: