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
feat(Progress stepper): Updated to use render prop for popover #7190
Conversation
Preview: https://patternfly-react-pr-7190.surge.sh A11y report: https://patternfly-react-pr-7190-a11y.surge.sh |
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! Other than the below comment, I opened issue #7200 after noticing something when testing the preview build (unrelated to this PR/issue)
/** @hide Forwarded reference to title container */ | ||
innerRef?: React.Ref<any>; | ||
/** Forwards the stepRef to rendered function. Use this prop to add a popover to the step.*/ | ||
render?: ({ stepRef }: { stepRef: React.RefObject<any> }) => React.ReactNode; |
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.
Nitpick that I wouldn't block approval over, but I was wondering if a prop name along the lines of renderPopover
or something might work here? Mainly thinking of making the prop name more specific.
packages/react-core/src/components/ProgressStepper/ProgressStep.tsx
Outdated
Show resolved
Hide resolved
packages/react-core/src/components/ProgressStepper/ProgressStep.tsx
Outdated
Show resolved
Hide resolved
I'm noticing that the keyboard accessibility is still missing from the example with a popover. Did you have a chance to peek at that? |
515d18d
to
5f6062a
Compare
id="popover-step2" | ||
titleId="popover-step2-title" | ||
aria-label="completed step with popover, step with danger" | ||
popoveRender={({ stepRef }) => ( |
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.
Missing 'r' here is why the build is failing atm.
Your changes have been released in:
Thanks for your contribution! 🎉 |
What: Closes #6658