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

TS migrate CardList Component #6564

Merged
merged 5 commits into from
Jun 9, 2022
Merged

Conversation

Mousticke
Copy link
Contributor

@Mousticke Mousticke commented Jun 4, 2022

Description

Migrate CardList component from js to tsx

  • Rename file from JS to TSX
  • Add typed props
  • [N/A] Add typed styled components

Analysis

Components / pages using CardListcomponent
BeaconChainActions ⚠️

  • datapoints → alt props needed
  • reads → OK

EthExchanges

  • exchanges→ alt props needed
  • walletProviders→ alt props needed

StableCoinAccordion ✔️

UpgradeArticles ✔️

bug-bounty ⚠️

  • clients→ alt props needed
  • specs→ OK

get-eth ⚠️

  • tokenSwaps→ alt props needed
  • decentralizedExchnages→ alt props needed
  • safetyArticles→ OK

deposit-contrat

  • addressSources → alt props needed

eth ✔️

wallets/index ✔️

Test new type

const testCarListArray: Array<CardListItem> = [
  {
    title:"title 1",
    description: "description 1",
    caption: "caption 1",
    link: "link 1",
    id: 0,
    image: "test",
    alt: "test"
  }, // OK
  {
    title:"title 1",
    description: "description 1",
    caption: "caption 1",
    link: "link 1",
    id: 0,
    alt: "test"
  }, // NOK IMAGE IS MISSING
  {
    title:"title 1",
    description: "description 1",
    caption: "caption 1",
    link: "link 1",
    id: 0,
    image: "test",
  }, // NOK ALT IS MISSING
  {
    title:"title 1",
    description: "description 1",
    caption: "caption 1",
    link: "link 1",
    id: 0,
  } // OK
]

Extra

GatsbyImage requires the alt props provided for an image or at least an empty string. So in the CardList component, if the image prop has been provided, then the alt props has to be passed too. I put in this way. If you do not agree with that, I can always give you your preferred way.

Related Issue

Related to Epic: TypeScript rollout

Mousticke added 3 commits June 3, 2022 21:36
add typed props for the card list and switch from
js file to typescript react component

Refs: ethereum#6392
the card list components has been migrated to typescript
react component. For a given card list item, if the image is provided,
then the alt props must be passed as GastbyImage requires the alt props

Refs: ethereum#6392
@gatsby-cloud
Copy link

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

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.

I like this, will help with our a11y issues. Good job @Mousticke 💯

=====
Not a big fan of the implementation aspects since its a bit hard to understand from a dx point of view but I guess there is not much we can do about that 🤷🏼‍♂️ will try to keep this in mind for other image implementations in other components.

src/components/CardList.tsx Outdated Show resolved Hide resolved
Mousticke added 2 commits June 7, 2022 19:20
define the image type in types.ts and set forbid utility type to more
a more generic name

Refs: ethereum#6392
@Mousticke Mousticke requested a review from pettinarip June 7, 2022 18:28
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.

@Mousticke this is great, thanks for the deep analysis there. While we migrate those other files, those errors are going to pop up and we can fix those missing alts. Good job 💪🏼

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

@all-contributors please add @Mousticke for code

@allcontributors
Copy link
Contributor

@pettinarip

@Mousticke already contributed before to code

@Mousticke Mousticke deleted the ts-migrate-cardlist branch June 9, 2022 23:48
@minimalsm minimalsm mentioned this pull request Jun 13, 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.

3 participants