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: migrate community components to typescript #2759

Merged
merged 13 commits into from
Mar 19, 2024
Merged

feat: migrate community components to typescript #2759

merged 13 commits into from
Mar 19, 2024

Conversation

devilkiller-ag
Copy link
Member

Description
This PR migrates the following components to TypeScript

  • community

Related issue(s)

#2636

Copy link

netlify bot commented Mar 12, 2024

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 1e61a1c
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/65f912ff740e660008432e8b
😎 Deploy Preview https://deploy-preview-2759--asyncapi-website.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.

@devilkiller-ag devilkiller-ag marked this pull request as draft March 12, 2024 05:57
@devilkiller-ag devilkiller-ag marked this pull request as ready for review March 12, 2024 08:37
Comment on lines 52 to 53
<Link href={link} target='_blank'>
<a target={link.includes('http') ? '_blank' : undefined}>
Copy link
Member

@anshgoyalevil anshgoyalevil Mar 13, 2024

Choose a reason for hiding this comment

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

Please follow this guide to either merge anchor with Link or add legacyBehavior prop to Link component.

Do this for all such usages in this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 19 to 20
// eslint-disable-next-line no-unused-vars, unused-imports/no-unused-vars
className = ''
Copy link
Member

Choose a reason for hiding this comment

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

You can include className prop to add className to the top-level div below.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 14 to 15
// eslint-disable-next-line no-unused-vars, unused-imports/no-unused-vars
className = ''
Copy link
Member

Choose a reason for hiding this comment

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

You can include className prop to add className to the top-level div below

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -18,7 +18,7 @@ interface HeaderProps {
export default function Header({
// eslint-disable-next-line no-unused-vars, unused-imports/no-unused-vars
className = ''
}: HeaderProps): JSX.Element {
}: HeaderProps): React.ReactNode {
Copy link
Member Author

Choose a reason for hiding this comment

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

@anshgoyalevil We should not use React.ReactNode here because it's being used inside the JSX in Hero.tsx. Due to this, using React.ReactNode is causing a build error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have changed this back to JSX.Element for now, let me know if there is any other option.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe you should then remove it all together, as suggested by Akshat

*/
export default function Hero({
className = ''
}: HeroProps): React.ReactNode {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}: HeroProps): React.ReactNode {
}: HeroProps) {

Don't specify the return type of functional components, that are exported by default. It will be taken automatically by Typescript.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Comment on lines 43 to 44

>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
>
>

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

*/
export default function Header({
className = ''
}: HeaderProps): JSX.Element {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}: HeaderProps): JSX.Element {
}: HeaderProps) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

btnText,
btnBg,
link
}: SmallHomeCardProp): React.ReactNode {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
}: SmallHomeCardProp): React.ReactNode {
}: SmallHomeCardProp) {

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

icon,
tagline,
taglineBg,
type = 'large',
Copy link
Member

Choose a reason for hiding this comment

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

define enum for this type in the types/community folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

* @param {string} props.btnBg - The background color for the button.
* @param {string} props.link - The link for the button.
*/
export default function SmallHomeCard({
Copy link
Member

Choose a reason for hiding this comment

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

Why the component has been named as SmallHomeCard?

Copy link
Member Author

Choose a reason for hiding this comment

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

This name is used in the master branch. But I think it will be more accurate to name it Card. Also, it's not used anywhere in the master branch except for writing its own test.

btnText,
link,
className
} : HomeCardProp) : React.ReactNode {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} : HomeCardProp) : React.ReactNode {
} : HomeCardProp) {

No need to define return type for the functional components.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for this. I have rectified this now.

@anshgoyalevil anshgoyalevil merged commit ad2d507 into asyncapi:migrate-ts Mar 19, 2024
13 checks passed
@devilkiller-ag devilkiller-ag deleted the ag-ts-community branch March 19, 2024 04:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants