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

feat(TimePicker): add typescript typings #13206

Merged
merged 6 commits into from
Feb 24, 2023
Merged

feat(TimePicker): add typescript typings #13206

merged 6 commits into from
Feb 24, 2023

Conversation

GalvinGao
Copy link
Contributor

@GalvinGao GalvinGao commented Feb 22, 2023

Closes #12563

Adds TypeScript typings to TimePicker and TimePickerSelect.

Changelog

New

  • Adds TypeScript typings to TimePicker and TimePickerSelect.

Changed

N/A

Removed

N/A

Testing / Reviewing

By using the storybook, the maintainer may navigate to /?path=/story/components-timepicker--default on their local storybook instance to see that the component successfully renders with the provided stories. A screenshot is attached for reference.

image

@GalvinGao GalvinGao requested a review from a team as a code owner February 22, 2023 07:32
@netlify
Copy link

netlify bot commented Feb 22, 2023

Deploy Preview for carbon-components-react ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 8b2eb10
🔍 Latest deploy log https://app.netlify.com/sites/carbon-components-react/deploys/63f8be9769fba10008c41346
😎 Deploy Preview https://deploy-preview-13206--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.

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.

Hey @GalvinGao 👋🏼
Thanks for getting this PR up!

Mostly LGTM, looks like TimePickerSelect changes are being picked up as a file deletion/addition, @GalvinGao did you use the gh mv script as per described in the Steps to provide baseline type definitions for components ? if not, could you? 🙏🏻

@GalvinGao
Copy link
Contributor Author

Hi @francinelucca!

Thanks for reviewing this PR. I've corrected the copyright part & also added myself to the all-contributors :D

However for the deletion+addition issue, I've tried to use git mv packages/react/src/components/TimePickerSelect/TimePickerSelect.js packages/react/src/components/TimePickerSelect/TimePickerSelect.tsx and reapplied my changes, but git still decided to choose this as a deletion+addition. My guess is that the changeset of that particular file is too big to the point that git diff decided to just treat it as a complete rewrite. Any other tips on how can I achieve the rename effect?

@GalvinGao GalvinGao requested review from francinelucca and removed request for tay1orjones February 22, 2023 17:36
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.

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.

Looks good, thanks for making that change 👍

@kodiakhq kodiakhq bot merged commit 0ab5fc1 into carbon-design-system:main Feb 24, 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 TimePicker, TimePickerSelect
3 participants