Skip to content
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

fix(popover): Deprecate PopoverAlignment type values #16346

Conversation

2nikhiltom
Copy link
Contributor

@2nikhiltom 2nikhiltom commented May 8, 2024

Closes #16326 #16467

In #14654 all Popover-based components were unified to use new values for the align prop.

This PR does the below

  • Deprecates old values so we can ensure consumers know to stop using them.

  • PopoverAlignment is now a union of both : PopoverAlignment = DeprecatedPopoverAlignment | NewPopoverAlignment;

  • Adds a custom validator function for proptypes that pops a warning when a deprecated value is used

Changelog

New

Adds deprecateValuesWithin function to warn when deprecated value is used for align prop
Pulls the old values out into their own type : DeprecatedPopoverAlignment and marks it as deprecated

Testing / Reviewing

Pass any old value for align pop in Popover component and verify that console warns about deprecated usage

<div>
      <Popover open={true} align="top-left" autoAlign></Popover>;
 </div>

Copy link

netlify bot commented May 8, 2024

Deploy Preview for v11-carbon-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 87d15d3
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/664462994a764500082bfe0e
😎 Deploy Preview https://deploy-preview-16346--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@2nikhiltom 2nikhiltom marked this pull request as ready for review May 8, 2024 11:13
@2nikhiltom 2nikhiltom requested review from a team as code owners May 8, 2024 11:13
@2nikhiltom 2nikhiltom changed the title fix(popover wip): custom fn to warn,deprecate old values,and unite both fix(popover): Deprecate PopoverAlignment type values May 8, 2024
Copy link
Contributor

@preetibansalui preetibansalui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few minor changes only, rest is looking fine to me.

packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
packages/react/src/prop-types/deprecateValuesWithin.js Outdated Show resolved Hide resolved
Copy link
Contributor

@preetibansalui preetibansalui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really great! I have a couple suggestions on the signature of deprecateValuesWithin. Let me know what you think

packages/react/src/prop-types/deprecateValuesWithin.js Outdated Show resolved Hide resolved
packages/react/src/prop-types/deprecateValuesWithin.js Outdated Show resolved Hide resolved
packages/react/src/components/Popover/index.tsx Outdated Show resolved Hide resolved
@2nikhiltom
Copy link
Contributor Author

Hey @tay1orjones !
Thanks for the enhancement suggestions 💯
I have changed the signature of deprecateValuesWithin and is ready for the review

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff!

@tay1orjones tay1orjones added this pull request to the merge queue May 15, 2024
Merged via the queue into carbon-design-system:main with commit b62c6b6 May 15, 2024
20 checks passed
@2nikhiltom 2nikhiltom linked an issue May 16, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants