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

migrate YouTube component to Typescript #6499

Merged
merged 4 commits into from
Jun 1, 2022
Merged

migrate YouTube component to Typescript #6499

merged 4 commits into from
Jun 1, 2022

Conversation

motemotech
Copy link

@motemotech motemotech commented May 29, 2022

Description

  • I migrated the YouTube component to TypeScript
  • There are very small changes to that.

Related Issue

#6392

@gatsby-cloud
Copy link

gatsby-cloud bot commented May 29, 2022

Gatsby Cloud Build Report

ethereum-org-website-dev

🎉 Your build was successful! See the Deploy preview here.

Build Details

View the build logs here.

🕐 Build time: 13m

Performance

Lighthouse report

Metric Score
Performance 🔶 23
Accessibility 💚 100
Best Practices 💚 100
SEO 💚 92

🔗 View full report

Copy link
Contributor

@minimalsm minimalsm left a comment

Choose a reason for hiding this comment

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

Hey @motemotech, thanks for the PR 💪😎

We should add type checks on the props here using an interface and explicitly declare what the function returns (a React functional component)

e.g.

export interface IProps {
  // props and types go here
}
const YouTube: (ADD A RETURN TYPE HERE) = ({ id, start = "0", title }) => {
...
}

Here is a simple example of how that might look.

@motemotech
Copy link
Author

Hi! Thank you @minimalsm !!
Let me fix that!

@samajammin
Copy link
Member

@motemotech here's some documentation on this: https://github.com/ethereum/ethereum-org-website/blob/dev/docs/typescript.md

@motemotech
Copy link
Author

@samajammin
Thank you! I understand what I have to do.

@motemotech
Copy link
Author

I've just changed my code.

Copy link
Member

@pettinarip pettinarip 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 @motemotech thanks for the contribution!

Copy link
Contributor

@minimalsm minimalsm left a comment

Choose a reason for hiding this comment

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

Hey @motemotech, the start prop should be optional as we only pass it when we want a YouTube video to start AFTER 0 seconds.


export interface IProps {
id: string
start: string
Copy link
Contributor

Choose a reason for hiding this comment

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

The start prop should be optional here.

Copy link
Author

Choose a reason for hiding this comment

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

Is this correct?
start?: string

Copy link
Member

Choose a reason for hiding this comment

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

@motemotech I just pushed a commit to make start and title as optional, and I gave them a default value.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you so much!

@pettinarip pettinarip merged commit 90032d6 into ethereum:dev Jun 1, 2022
@pettinarip
Copy link
Member

@all-contributors please add @motemotech for code

@allcontributors
Copy link
Contributor

@pettinarip

I've put up a pull request to add @motemotech! 🎉

@minimalsm
Copy link
Contributor

@motemotech be sure to join the discord if you are interested in contributing further to the project or have any questions for the team. And we've just released our 2022 POAPs so remember to claim yours also 🥳!

@motemotech
Copy link
Author

motemotech commented Jun 2, 2022

I'm Nico! I already joined the discord!
@minimalsm

@wackerow wackerow mentioned this pull request Jun 3, 2022
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.

5 participants