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: migrate components/Callout.js to ts #6540

Merged
merged 5 commits into from
Jun 9, 2022

Conversation

byhow
Copy link
Contributor

@byhow byhow commented Jun 2, 2022

Description

Migrate Callout.js, which also includes its imports of Translation.js and Emoji.js

Related Issue

#6392

@gatsby-cloud
Copy link

gatsby-cloud bot commented Jun 2, 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: 28m

Performance

Lighthouse report

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

🔗 View full report

@byhow byhow force-pushed the ts-migration-components branch from 4cd417d to 994c084 Compare June 3, 2022 04:32
text: string
}

// const StyledTweomoji: React.FC<Twemoji>

const StyledEmoji = styled(Twemoji)`
& > img {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Twemoji's prop has no .size property. In order to pass it in, it seems like it needs to be a way to override its FC signature

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 extend the styled component by doing something like this:

const StyledEmoji = styled(Twemoji)<{ size: number }>`
  ...
`

@@ -46,7 +46,16 @@ const Content = styled.div`
height: 100%;
`

const Callout = ({
interface IProps {
image: string
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
image: string
image?: string

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated

text: string
}

// const StyledTweomoji: React.FC<Twemoji>

const StyledEmoji = styled(Twemoji)`
& > img {
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 extend the styled component by doing something like this:

const StyledEmoji = styled(Twemoji)<{ size: number }>`
  ...
`

@pettinarip pettinarip self-assigned this Jun 3, 2022
@byhow byhow marked this pull request as ready for review June 4, 2022 01:54
@byhow byhow requested a review from pettinarip June 4, 2022 22:22
text: string
}

// const StyledTweomoji: React.FC<Twemoji>
Copy link
Member

Choose a reason for hiding this comment

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

✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah good catch! updated

@byhow byhow force-pushed the ts-migration-components branch from 20c42ae to ff1622e Compare June 7, 2022 00:18
@byhow byhow requested a review from samajammin June 7, 2022 00:19
@byhow
Copy link
Contributor Author

byhow commented Jun 7, 2022

ah, gatsby cloud says I don't have permission to view the failed build. might need someone else to take a preliminary look

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.

Looking good @byhow 💪🏼 left a couple of minor comments there to be done.


const StyledEmoji = styled(Twemoji)`
interface IProp extends MarginProps {
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
interface IProp extends MarginProps {
export interface IProps extends MarginProps {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cool, updated

@@ -13,7 +18,7 @@ const StyledEmoji = styled(Twemoji)`
${margin}
`

const Emoji = ({ size = 1.5, text, ...props }) => {
const Emoji: React.FC<IProp> = ({ size = 1.5, text, ...props }) => {
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
const Emoji: React.FC<IProp> = ({ size = 1.5, text, ...props }) => {
const Emoji: React.FC<IProps> = ({ size = 1.5, text, ...props }) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

@@ -46,7 +46,16 @@ const Content = styled.div`
height: 100%;
`

const Callout = ({
interface IProps {
Copy link
Member

Choose a reason for hiding this comment

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

Just for convention

Suggested change
interface IProps {
export interface IProps {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

base props exported

@byhow
Copy link
Contributor Author

byhow commented Jun 8, 2022

@pettinarip Thanks for the detailed suggestion! Changes are resolved

@byhow byhow requested a review from pettinarip June 8, 2022 22:47
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.

Great! Thanks @byhow looks good 💪🏼

@pettinarip
Copy link
Member

@byhow 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 🥳!

@pettinarip pettinarip merged commit ecc7ac3 into ethereum:dev Jun 9, 2022
@pettinarip
Copy link
Member

@all-contributors please add @byhow for code

@allcontributors
Copy link
Contributor

@pettinarip

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

@Mousticke Mousticke mentioned this pull request Jun 9, 2022
@Mousticke
Copy link
Contributor

@pettinarip @byhow : I am sorry but I noticed that a bit late.
size has a default value of 1.5. But still, it's required by the IProps interface.

→ The size props should be optional if it has a default value

@pettinarip
Copy link
Member

Good catch @Mousticke! yes, we should fix that 👍🏼

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.

4 participants