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

refactor: move ToggleSkeleton to TypeScript #13451

Merged

Conversation

gerzonc
Copy link
Contributor

@gerzonc gerzonc commented Apr 3, 2023

Closes #12565

Migrates ToggleSkeleton to TypeScript

Changelog

New

  • Adds type ToggleSkeletonProps for ToggleSkeleton
  • Adds partial type for defaultProps

Changed

  • ToggleSkeleton is now a TypeScript file
  • Ternary for aria-label now checks for undefined instead of null if there is a labelText present

Removed

  • {{removed thing}}

Testing / Reviewing

{{ Add descriptions, steps or a checklist for how reviewers can verify this PR works or not }}

@gerzonc gerzonc requested a review from a team as a code owner April 3, 2023 00:05
@github-actions
Copy link
Contributor

github-actions bot commented Apr 3, 2023

DCO Assistant Lite bot All contributors have signed the DCO.

@netlify
Copy link

netlify bot commented Apr 3, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ec2e2ab
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/642a18486f4190000872a651
😎 Deploy Preview https://deploy-preview-13451--carbon-components-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 settings.

@netlify
Copy link

netlify bot commented Apr 3, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit ec2e2ab
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/642a18485dfa2e00082badd9
😎 Deploy Preview https://deploy-preview-13451--carbon-elements.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 settings.

@gerzonc
Copy link
Contributor Author

gerzonc commented Apr 3, 2023

I have read the DCO document and I hereby sign the DCO.

@netlify
Copy link

netlify bot commented Apr 3, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 847205c
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/642aeaf32f763b0008f3026f
😎 Deploy Preview https://deploy-preview-13451--carbon-components-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 settings.

@netlify
Copy link

netlify bot commented Apr 3, 2023

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 847205c
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/642aeaf3307ebc00089b4847
😎 Deploy Preview https://deploy-preview-13451--carbon-elements.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 settings.

@alisonjoseph
Copy link
Member

I think this may be an existing bug, but I quickly added a story for ToggleSkeleton locally so I can test and it looks like this 🤔

Screenshot 2023-04-03 at 7 57 37 AM

@gerzonc
Copy link
Contributor Author

gerzonc commented Apr 3, 2023

I think this may be an existing bug, but I quickly added a story for ToggleSkeleton locally so I can test and it looks like this 🤔

Screenshot 2023-04-03 at 7 57 37 AM

Yeah, that's how it looks like from the main branch. If there is some kind of design available just to see how it should look like, I would be glad to fix it right away. :)

@francinelucca
Copy link
Collaborator

Ran on main branch and confirmed broken styling:
image

Opened up new issue #13459.
Other than that this LGTM , @alisonjoseph should we add that issue as blocker for this one or is this good to merge? 🤔

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

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

Approving this, we can handle the visual bug and adding to storybook in a separate issue/PR 😄

@gerzonc looks like there is a small merge conflict in the readme

Copy link
Collaborator

@francinelucca francinelucca left a comment

Choose a reason for hiding this comment

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

@gerzonc thanks for adding this, looks good! 🚀
After you solve the Readme.md merge conflicts and this should be good to go 🙏🏻

@kodiakhq kodiakhq bot merged commit 19a70c2 into carbon-design-system:main Apr 3, 2023
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.

Add TypeScript types to ToggleSkeleton
3 participants