chore(Page): add aria label prop to page subcomponents explicitly #7822
Conversation
|
Preview: https://patternfly-react-pr-7822.surge.sh A11y report: https://patternfly-react-pr-7822-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
This looks good! I had a couple of nits below (1 question and 1 suggested change), let me know what you think!
| hasShadowBottom?: boolean; | ||
| /** Flag indicating if the PageBreadcrumb has a scrolling overflow */ | ||
| hasOverflowScroll?: boolean; | ||
| /** Adds an accessible name to the breadcrumb section. Required when the has overflow scroll prop is set to true. */ |
There was a problem hiding this comment.
Nit: I know prop descriptions should use sentence case, but I'm wondering if that should apply when referencing other props?
There was a problem hiding this comment.
I'm not positive about that myself. I think that having other props in camelCase makes things a bit more clear, but it does conflict with the sentence case prop descriptions guidance as you mentioned. I realize now that some of the other prop descriptions here are already using camelCase.
@tlabaj do you have any thoughts on this?
There was a problem hiding this comment.
Same would apply for things like "onChange" and whether that should be written as "on change..." instead within the context of prop descriptions.
There was a problem hiding this comment.
I think for a prop name it is ok to deviate from the sentence case standard.
Co-authored-by: Eric Olkowski <70952936+thatblindgeye@users.noreply.github.com>
tlabaj
left a comment
tlabaj
left a comment
There was a problem hiding this comment.
Looks good! Just one comment to be applied to all warnings.
| React.useEffect(() => { | ||
| if (hasOverflowScroll && !ariaLabel) { | ||
| /* eslint-disable no-console */ | ||
| console.warn('An accessible aria-label is required when hasOverflowScroll is set to true.'); |
There was a problem hiding this comment.
I would add the component name to warnings to make it easier for consumer to identify what component needs attention.
e.g. "PageBreadcrumb: An accessible aria-label is required when hasOverflowScroll is set to true.'
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
5 participants
What: Closes #7350
Additional issues: