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

Extract the chip variant of radio-box-group to a different component #436

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

aswasif007
Copy link
Contributor

@aswasif007 aswasif007 commented Aug 26, 2024

Description

Presently we have a chip variant of the RadioBoxGroup component. We are in need for a small size of this chip variant. But the component RadioBoxGroup does not provide any size prop, nor does its other variant need any.

So in this PR we are extracting the chip variant to a separate component RadioGroupChip and adding a size prop with small and medium values.

output

Steps to Test

  1. Pull down PR.
  2. npm run dev.
  3. Open http://localhost:6006/?path=/docs/radiogroupchip--docs

Copy link

netlify bot commented Aug 26, 2024

Deploy Preview for vip-design-system-components ready!

Name Link
🔨 Latest commit 68b53d6
🔍 Latest deploy log https://app.netlify.com/sites/vip-design-system-components/deploys/66cda165f30f6f000830c50d
😎 Deploy Preview https://deploy-preview-436--vip-design-system-components.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.

Copy link
Contributor

@djalmaaraujo djalmaaraujo left a comment

Choose a reason for hiding this comment

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

The implementation looks good, but you are duplicating the RadioGroup component, which already supports the Chip variation. Is there a reason we are recreating the component without removing the Chip functionality from the original?

Could adding the size property to the RadioBoxGroup component be a simpler solution? If you decide to go with a new component for the chip, we need to at least refactor RadioBoxGroup to remove the already-added chip feature.

PS: Since I believe you chose the "new chip component," please ensure that you reuse code from the RadioBoxGroup; otherwise, we might have different or unexpected experiences.

Milestone suggestion:

1 - Remove the current chip feature from RadioBoxGroup
2 - Release the new version of this library
3 - Open a vip-dashboard pull request with both replacements of the currently chip usage, and the new vip-design-system version

Feel free to let me know how I can help.

@aswasif007
Copy link
Contributor Author

Is there a reason we are recreating the component without removing the Chip functionality from the original?

I will be removing the original chip variant, but was waiting on your opinion first 😄

Copy link
Contributor

@djalmaaraujo djalmaaraujo left a comment

Choose a reason for hiding this comment

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

Thanks for working on these changes @aswasif007

@djalmaaraujo djalmaaraujo merged commit 03592b0 into trunk Aug 27, 2024
8 checks passed
@djalmaaraujo djalmaaraujo deleted the add/radio-group-chip branch August 27, 2024 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants