fix: BreadcrumbItem aria-current PropType fix #15052
Conversation
The PropType definition for 'aria-current' is to broad. Update the PropType to match AriaAttributes['aria-current'] BREAKING CHANGE: PropType is changed to be more specific
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
This will require a snapshot update but I am not certain how best to alter it, is there a command or a manual process? |
|
@Tresau-IBM you should able to run |
Thank you, will address merge conflict and update snapshot, thank you |
|
|
||
| export interface BreadcrumbItemProps | ||
| extends React.HTMLAttributes<HTMLLIElement> { | ||
| 'aria-current'?: AriaAttributes['aria-current']; |
There was a problem hiding this comment.
The TypeScript types aren't bound by semver right now and are provided on an as-is basis. I think it's fine to specify this here. It won't be classified as a breaking change.
| BreadcrumbItem.propTypes = { | ||
| 'aria-current': PropTypes.oneOfType([ | ||
| PropTypes.bool, | ||
| PropTypes.oneOf([ | ||
| 'false', | ||
| 'true', | ||
| 'page', | ||
| 'step', | ||
| 'location', | ||
| 'date', | ||
| 'time', | ||
| ] as const), | ||
| ]), |
There was a problem hiding this comment.
I'm not sure this warrants a breaking change either. The spec is quite clear on what values are allowed, though there is one exception (I bolded below for emphasis)
The aria-current attribute is a token type. Any value not included in the list of allowed values SHOULD be treated by assistive technologies as if the value true had been provided. If the attribute is not present or its value is an empty string or undefined, the default value of false applies and the aria-current state MUST NOT be exposed by user agents or assistive technologies.
So technically any string is allowable and coalesces to true. You could argue these proptypes (plus the typings from React) are incorrect, but I don't think this presents much risk to have specific strings instead of allowing any string. If we get user feedback that some use case is missed, we can roll back this change very easily.
Head branch was pushed to by a user without write access
…5052) * feat: add types for Breadcrumbs * revert: undo PropType changes and supress TS error instead * fix: update 'aria-current' to match standards The PropType definition for 'aria-current' is to broad. Update the PropType to match AriaAttributes['aria-current'] BREAKING CHANGE: PropType is changed to be more specific * fix: update snapshot
Closes #15034
BREAKING CHANGE as it makes a PropType more specific.
The PropType for 'aria-current' in BreadcrumbItem is to broad, this change makes the definition more specific to match AriaAttributes['aria-current']. Depends on #15033 as this PR is branched from that issue.
Changelog
New
Changed
string, booltobool, 'false', 'true', 'page', 'step', 'location', 'date', 'time'Removed
Testing / Reviewing
Verify that the storybook has updated PropTypes
Verify tests pass