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

Replace deprecated Typography with Text component #17670

Open
georgewrmarshall opened this issue Feb 9, 2023 · 59 comments · Fixed by #18948, #18964, #19075, #19433 or #25017
Open

Replace deprecated Typography with Text component #17670

georgewrmarshall opened this issue Feb 9, 2023 · 59 comments · Fixed by #18948, #18964, #19075, #19433 or #25017
Labels
good first issue Good for newcomers INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-12.2.0 Issue or pull request that will be included in release 12.2.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 release-12.4.0 Issue or pull request that will be included in release 12.4.0 release-12.5.0 Issue or pull request that will be included in release 12.5.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-design-system All issues relating to design system in Extension

Comments

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented Feb 9, 2023

Description

In an effort to reduce duplication and replace our old design system components with new components it would be great to replace all <Typography /> with <Text /> components. This is a massive undertaking by itself and creating a single PR would be too large. Instead this issue could be broken up into multiple PRs.

Technical Details

  • Swap all Typography components with Text equivalent(make sure to check the component apis as they are slightly different)
  • boxProps should be migrated appropriately e.g. boxProps={{marginTop: 1}} => marginTop={1}
  • Some TextVariant have bold font weight options which can be used instead of fontWeight={FONT_WEIGHT.BOLD} => variant={TextVariant.bodyMdBold}
  • Check the Typography => Text component map below for the correct variant prop values

Typography => Text component map

<Typography variant={TypographyVariant.H1}> => <Text variant={TextVariant.displayMd}>
<Typography variant={TypographyVariant.H2}> => <Text variant={TextVariant.headingLg}>
<Typography variant={TypographyVariant.H3}> => <Text variant={TextVariant.headingMd}>
<Typography variant={TypographyVariant.H4}> => <Text variant={TextVariant.headingSm}>
<Typography variant={TypographyVariant.H5}> => <Text variant={TextVariant.bodyMd}>
<Typography variant={TypographyVariant.H6}> => <Text variant={TextVariant.bodySm}>
<Typography variant={TypographyVariant.Paragraph}> => <Text variant={TextVariant.bodyMd}
<Typography variant={TypographyVariant.H7}> => <Text variant={TextVariant.bodySm}>
<Typography variant={TypographyVariant.H8}> => <Text variant={TextVariant.bodyXs}>
<Typography variant={TypographyVariant.H9}> => <Text variant={TextVariant.bodyXs}>

Acceptance Criteria

  • All Typography components have been replaced with their Text equivalent
  • PR contains a minimum of 1 file and a maximum of 3 files
  • Add Before / After screenshots to your PR to ensure no visual regressions
  • Reference this issue in the description of your PR
  • All CI tests are passing jest, lint, e2e etc

PRs that don't meet the acceptance criteria may be closed.

References

Text component documentation

@georgewrmarshall georgewrmarshall added good first issue Good for newcomers area-UI Relating to the user interface. team-design-system All issues relating to design system in Extension labels Feb 9, 2023
@georgewrmarshall
Copy link
Contributor Author

Please take note that the current Text documentation will be out of date until #17674 has been merged 🙏

@itsAfnanAlam
Copy link

Hi, can I work on this?

@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Feb 10, 2023

Hi @itsAfnanAlam, you're welcome to submit a PR toward this issue please title it Part of #17670: Replace Typography with Text component

@itsAfnanAlam
Copy link

@georgewrmarshall, working on it. Thanks!

@Aathirajan
Copy link

hi @georgewrmarshall is it up for grabs? if yes can I?

@itsAfnanAlam
Copy link

@Aathirajan I am working on it.

@Aathirajan
Copy link

Great! Glad you mentioned it.

itsAfnanAlam added a commit to itsAfnanAlam/metamask-extension that referenced this issue Feb 11, 2023
@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Feb 14, 2023

Hey @itsAfnanAlam and @Aathirajan, this issue is intended to be broken up in to multiple PRs for easy review. I'm listing files engineers would like to, or are working on in the description under "Files being currently converted". You could both comment on the files you would like to convert and I'll add them to the list. See example PR here #17753

@sammaji
Copy link

sammaji commented Feb 14, 2023

@georgewrmarshall can you point out some files that need change first and we'll look into them?
Here's some that I found:

  • \ui\components\app\add-network\add-network.js
  • \ui\components\app\advanced-gas-fee-popover\advanced-gas-fee-defaults\advanced-gas-fee-defaults.js
  • \ui\components\app\advanced-gas-fee-popover\advanced-gas-fee-gas-limit\advanced-gas-fee-gas-limit.js
  • \ui\components\app\approve-content-card\approve-content-card.js

We could work on these first.

@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Feb 14, 2023

Hey @samyabrata-maji, some of those are being worked on see "Files currently being converted" at the bottom section in this issue.

Screenshot 2023-02-14 at 3 41 52 PM

  • \ui\components\app\add-network\add-network.js in-progress 🚫
  • \ui\components\app\advanced-gas-fee-popover\advanced-gas-fee-defaults\advanced-gas-fee-defaults.js available to work on ✅
  • \ui\components\app\advanced-gas-fee-popover\advanced-gas-fee-gas-limit\advanced-gas-fee-gas-limit.js available to work on ✅
  • \ui\components\app\approve-content-card\approve-content-card.js in-progress 🚫

I'll add your name to the list for the available files in that list thanks!

@spiritanand
Copy link
Contributor

Hey @georgewrmarshall are all the files being worked by other contributors? Could I contribute in any way?

@sammaji
Copy link

sammaji commented Feb 15, 2023

Hey @georgewrmarshall are all the files being worked by other contributors? Could I contribute in any way?
There are other files that require changes.

Screenshot 2023-02-15 181600

@spiritanand Just make sure to let us know which ones you would be working on 👍

@spiritanand
Copy link
Contributor

@samyabrata-maji I did not understand. Are the ones in the ss available for me to work on?

@Yomna-J
Copy link
Contributor

Yomna-J commented Feb 15, 2023

@georgewrmarshall A quick update:
I've been working on the following and changes have been made. However, I'm unable to add screenshots as "This conversation has been locked and limited to collaborators."

@sammaji
Copy link

sammaji commented Feb 15, 2023

@samyabrata-maji I did not understand. Are the ones in the ss available for me to work on?

Some of them are available. There are other files too that are not in the ss.

@georgewrmarshall
Copy link
Contributor Author

georgewrmarshall commented Feb 15, 2023

Hey @spiritanand, you can do a search for <Typography in your code editor. Excluding the Typography folder itself(ui/components/ui/typography) There are 119 files you can choose from that need to be converted. So far the bottom section "Files currently being converted" at the bottom of this issue description is what is being worked on. So you can comment here on what file you would like to work on and I will add it to the list under your name.

  1. Search <Typography
    Screenshot 2023-02-15 at 8 05 01 AM

  2. Pick a file that has not been taken and IS NOT in this list

Screenshot 2023-02-15 at 8 10 26 AM

  1. Comment on this issue the file that you'd like to convert and I will add it to the description under your name

@SawanKumarBhagat
Copy link

Hey! @georgewrmarshall I worked on these files and made a pr #17766 can you check them please?
metamask-extension\ui\components\app\asset-list\asset-list.js
metamask-extension\ui\components\app\beta-header\index.js
metamask-extension\ui\components\app\cancel-speedup-popover\cancel-speedup-popover.js
metamask-extension\ui\components\app\confirm-page-container\confirm-page-container.component.js
metamask-extension\ui\components\app\confirm-page-container\confirm-page-container-content\confirm-page-container-content.component.js

@ayushj1910
Copy link
Contributor

Hey @georgewrmarshall ,
I would like to work on this issue.

@metamaskbot
Copy link
Collaborator

Missing release label release-12.2.0 on issue. Adding release label release-12.2.0 on issue, as issue is linked to PR #25176 which has this release label.

@metamaskbot
Copy link
Collaborator

Missing release label release-12.2.0 on issue. Adding release label release-12.2.0 on issue, as issue is linked to PR #25050 which has this release label.

@metamaskbot
Copy link
Collaborator

Missing release label release-12.2.0 on issue. Adding release label release-12.2.0 on issue, as issue is linked to PR #25019 which has this release label.

@metamaskbot
Copy link
Collaborator

Missing release label release-12.2.0 on issue. Adding release label release-12.2.0 on issue, as issue is linked to PR #25017 which has this release label.

@metamaskbot
Copy link
Collaborator

Missing release label release-12.2.0 on issue. Adding release label release-12.2.0 on issue, as issue is linked to PR #25149 which has this release label.

@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 9, 2024
@gauthierpetetin gauthierpetetin added release-12.4.0 Issue or pull request that will be included in release 12.4.0 release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 11, 2024
@metamaskbot metamaskbot added the release-12.6.0 Issue or pull request that will be included in release 12.6.0 label Sep 25, 2024
vinnyhoward pushed a commit that referenced this issue Sep 25, 2024
…ent.js (#27053)

## **Description**

This pull request replaces the `Typography` component with the new
`Text` component in the file
`ui/pages/confirmations/components/gas-timing/gas-timing.component.js`.
This change is part of the ongoing effort to migrate from the old
`Typography` component to the new `Text` component across the MetaMask
extension codebase.

The main improvements in this change are:
1. Consistency with the new design system
2. Better maintainability by using the latest component library
3. Improved performance due to the optimized `Text` component

Devin Run Link:
https://preview.devin.ai/devin/d0382f35dd004ffba790f08bbd5ffc9c

[![Open in GitHub
Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/27053?quickstart=1)

## **Related issues**

Partially Fixes:
#17670

## **Manual testing steps**

1. Go to the latest build of storybook in this PR
2. Navigate to the GasTiming component
3. Verify that the component renders correctly with the new Text
component
4. Check that all text styles and layouts are preserved

## **Screenshots/Recordings**

### **Before**

<img width="757" alt="Screenshot 2024-09-11 at 8 40 05 AM"
src="https://github.com/user-attachments/assets/ad1277fd-1e54-4a0d-a5d0-cc2ccb314c9a">

### **After**

<img width="1512" alt="Screenshot 2024-09-11 at 8 43 58 AM"
src="https://github.com/user-attachments/assets/e7d94fa3-caea-4af9-b525-ee5914dd9089">
<img width="1510" alt="Screenshot 2024-09-11 at 8 44 35 AM"
src="https://github.com/user-attachments/assets/84759078-fe2b-4cc1-be7e-29ef43eda8c5">


## **Pre-merge author checklist**

- [X] I've followed [MetaMask Coding
Standards](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/CODING_GUIDELINES.md).
- [X] I've completed the PR template to the best of my ability
- [X] I've included tests if applicable
- [X] I've documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [X] I've applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-extension/blob/develop/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **Pre-merge reviewer checklist**

- [ ] I've manually tested the PR (e.g. pull and build branch, run the
app, test code being changed).
- [ ] I confirm that this PR addresses all acceptance criteria described
in the ticket it closes and includes the necessary testing evidence such
as recordings and or screenshots.


If you have any feedback, you can leave comments in the PR and I'll
address them in the app!

---------

Co-authored-by: georgewrmarshall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers INVALID-ISSUE-TEMPLATE Issue's body doesn't match any issue template. release-12.2.0 Issue or pull request that will be included in release 12.2.0 release-12.3.0 Issue or pull request that will be included in release 12.3.0 release-12.4.0 Issue or pull request that will be included in release 12.4.0 release-12.5.0 Issue or pull request that will be included in release 12.5.0 release-12.6.0 Issue or pull request that will be included in release 12.6.0 team-design-system All issues relating to design system in Extension
Projects
None yet