-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(HeaderMenuItem): update prop #13375
feat(HeaderMenuItem): update prop #13375
Conversation
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-components-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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.
Thanks for this! Just a couple minor things to address
|
||
/** | ||
* Applies selected styles to the item if a user sets this to true and `aria-current !== 'page'`. | ||
* @deprecated Please use `isActive`. This will be removed soon in v12. |
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.
* @deprecated Please use `isActive`. This will be removed soon in v12. | |
* @deprecated Please use `isActive` instead. This will be removed in the next major release. |
|
||
/** | ||
* Applies selected styles to the item if a user sets this to true and `aria-current !== 'page'`. | ||
* @deprecated Please use `isActive`. This will be removed soon in v12. | ||
*/ | ||
isCurrentPage: PropTypes.bool, |
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.
This needs the deprecate()
prop types function helper applied here.
Here's an example of how it's used:
carbon/packages/react/src/components/ComboBox/ComboBox.tsx
Lines 736 to 740 in b0f39f3
light: deprecate( | |
PropTypes.bool, | |
'The `light` prop for `Combobox` has ' + | |
'been deprecated in favor of the new `Layer` component. It will be removed in the next major release.' | |
), |
Hey @tay1orjones, I have updated the PR. PTAL, Thanks! 😃 |
Hi @tay1orjones, as suggested by @tw15egan, I tried doing the same thing to |
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.
LGTM 👍 ✅
Closes #13369
Changelog
Changed
isActive
propisCurrentPage
in HeaderMenuItemTesting / Reviewing
To verify these changes are working, we can check to see if the existing storybook behavior is still working correctly.