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.

Sign up for GitHub

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

Jump to bottom

Badge - Convert to TypeScript #1991

Merged
merged 11 commits into from
May 9, 2024
Merged

Badge - Convert to TypeScript #1991

merged 11 commits into from
May 9, 2024

Conversation

chris-hut
Copy link
Contributor

@chris-hut chris-hut commented Mar 13, 2024 •
edited by alex-ju
Loading

📌 Summary

We love TypeScript! This PR converts the famous 🦡 component over to Typescript!

🛠️ Detailed description

With glint being enabled across the design-system - this PR converts a component over to Typescriptland!

🔗 External links

👀 Component checklist

💬 Please consider using conventional comments when reviewing this PR.

References

image

@vercel Vercel
Copy link

vercel bot commented Mar 13, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
hds-showcase ✅ Ready (Inspect) Visit Preview May 9, 2024 6:42pm
hds-website ✅ Ready (Inspect) Visit Preview May 9, 2024 6:42pm

natmegs
natmegs reviewed Mar 13, 2024
View reviewed changes
Copy link
Contributor

@natmegs natmegs left a comment

Choose a reason for hiding this comment

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

Awesome 🙌 🙌 could we remove the glint-nocheck in the template too?

valeriia-ruban
valeriia-ruban reviewed Mar 14, 2024
View reviewed changes
Copy link
Contributor

@valeriia-ruban valeriia-ruban left a comment

Choose a reason for hiding this comment

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

Looks great!
I've just added very minor thing to take a look at.

packages/components/src/template-registry.ts Outdated Show resolved Hide resolved
didoo
didoo reviewed Mar 14, 2024
View reviewed changes
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

Left a few comments, mainly for internal discussion.

.changeset/real-pets-work.md Outdated Show resolved Hide resolved
packages/components/src/components/hds/badge/index.ts Outdated Show resolved Hide resolved
packages/components/src/components/hds/badge/types.ts Outdated Show resolved Hide resolved
@WenInCode
Copy link
Contributor

@alex-ju / @didoo -- Chris is out on medical leave for a few weeks so I updated this branch. Could one of you give this another look when you have some time? Appreciate it!

didoo
didoo reviewed Mar 26, 2024
View reviewed changes
Copy link
Contributor

@didoo didoo left a comment

Choose a reason for hiding this comment

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

I have left a bunch of comments, but since I see differences between the conversion of this component in comparison with the previous ones, is better to wait for @alex-ju to come back from PTO (he has more context and knowledge)

packages/components/src/components/hds/badge/index.ts Outdated Show resolved Hide resolved
packages/components/src/components/hds/badge/index.ts Outdated Show resolved Hide resolved
packages/components/src/components/hds/badge/index.ts Outdated Show resolved Hide resolved
@didoo
Copy link
Contributor

didoo commented Mar 26, 2024 •
edited
Loading

@alex-ju / @didoo -- Chris is out on medical leave for a few weeks so I updated this branch. Could one of you give this another look when you have some time? Appreciate it!

@WenInCode see my comment above

since I see differences between the conversion of this component in comparison with the previous ones, is better to wait for @alex-ju to come back from PTO (he has more context and knowledge)

alex-ju
alex-ju reviewed Apr 4, 2024
View reviewed changes
Copy link
Member

@alex-ju alex-ju 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 getting this to the finish line @WenInCode! there are a few changes before we can get this merged, but let me know if you don't have the time to address them and I can take it over

packages/components/src/components/hds/badge/index.ts Outdated Show resolved Hide resolved
@alex-ju alex-ju changed the title TypeScript: Convert badge to Typescript Badge - Convert to TypeScript May 9, 2024
alex-ju
alex-ju approved these changes May 9, 2024
View reviewed changes
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Rebased and addressed the remaining feedback

@alex-ju alex-ju merged commit f7dd564 into main May 9, 2024
16 checks passed
@alex-ju alex-ju deleted the ts/badge branch May 9, 2024 18:48
@hashibot-hds hashibot-hds mentioned this pull request May 9, 2024
Merged
@alex-ju alex-ju mentioned this pull request May 14, 2024
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aklkv aklkv aklkv left review comments

@didoo didoo didoo left review comments

@WenInCode WenInCode WenInCode left review comments

@valeriia-ruban valeriia-ruban valeriia-ruban left review comments

@natmegs natmegs natmegs left review comments

@alex-ju alex-ju alex-ju approved these changes

Assignees
No one assigned
Labels
packages/components typescript
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@chris-hut @WenInCode @didoo @aklkv @alex-ju @valeriia-ruban @natmegs @Dhaulagiri @hashibot-hds