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: replace Typography with Text component in gas-timing.component.js #27053

Merged
merged 6 commits into from
Sep 25, 2024

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Sep 11, 2024

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

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

Screenshot 2024-09-11 at 8 40 05 AM

After

Screenshot 2024-09-11 at 8 43 58 AM Screenshot 2024-09-11 at 8 44 35 AM

Pre-merge author checklist

  • I've followed MetaMask Coding Standards.
  • I've completed the PR template to the best of my ability
  • I've included tests if applicable
  • I've documented my code using JSDoc format if applicable
  • I've applied the right labels on the PR (see labeling guidelines). 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!

Copy link
Contributor

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

Comment on lines -1 to -12
.typography.gas-timing {
color: var(--color-text-alternative);

.info-tooltip {
display: inline-block;
margin-inline-start: 4px;

path {
fill: var(--color-error-default);
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unused CSS and targeted the Typography component and InfoTooltip it shouldn't use component classes and InfoTooltip is no longer used in this file

Screenshot 2024-09-11 at 8 41 58 AM

Comment on lines +50 to +59
export const GasTooLowStory: Story = {
args: {
maxFeePerGas: '1', // Simulate low gas fee
maxPriorityFeePerGas: '1', // Simulate low priority fee
gasWarnings: {
maxPriorityFee: GAS_FORM_ERRORS.MAX_PRIORITY_FEE_TOO_LOW,
maxFee: GAS_FORM_ERRORS.MAX_FEE_TOO_LOW,
},
},
};
Copy link
Contributor

@georgewrmarshall georgewrmarshall Sep 11, 2024

Choose a reason for hiding this comment

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

Adding new story to display replacement Text component

Before

Screenshot 2024-09-11 at 8 40 05 AM

After

Screenshot 2024-09-11 at 8 43 58 AM Screenshot 2024-09-11 at 8 44 35 AM

@@ -26,7 +26,6 @@
@import 'edit-gas-fee-popover/edit-gas-tooltip/index';
@import 'edit-gas-popover/index';
@import 'gas-details-item/index';
@import 'gas-timing/index';
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing deleted stylesheet from confirmations style index

fontWeight={FontWeight.Bold}
color={TextColor.textAlternative}
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of changing color via the CSS we are doing using the style utility props

@georgewrmarshall georgewrmarshall marked this pull request as ready for review September 11, 2024 15:49
@georgewrmarshall georgewrmarshall requested a review from a team as a code owner September 11, 2024 15:49
@georgewrmarshall georgewrmarshall added team-design-system All issues relating to design system in Extension team-ai AI team (for the Devin AI bot) and removed external-contributor labels Sep 11, 2024
Copy link
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines +37 to +38
maxFeePerGas: '0',
maxPriorityFeePerGas: '0',
Copy link
Contributor

Choose a reason for hiding this comment

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

Fixes type error

Screenshot 2024-09-11 at 8 52 28 AM

@@ -4,6 +4,7 @@ import { Provider } from 'react-redux';
import configureStore from '../../../../store/store';
import GasTiming from './gas-timing.component';
import mockState from '../../../../../test/data/mock-state.json';
import { GAS_FORM_ERRORS } from '../../../../helpers/constants/gas';
Copy link
Contributor

Choose a reason for hiding this comment

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

Used in new GasTooLowStory

@metamaskbot
Copy link
Collaborator

Builds ready [8495d1e]
Page Load Metrics (1830 ± 76 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24722381677450216
domContentLoaded15782226180415675
load15862236183015876
domInteractive13124402612
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -146 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

Copy link

codecov bot commented Sep 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 70.11%. Comparing base (7dc6362) to head (8495d1e).
Report is 10 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #27053      +/-   ##
===========================================
- Coverage    70.15%   70.11%   -0.05%     
===========================================
  Files         1425     1426       +1     
  Lines        49653    49689      +36     
  Branches     13891    13902      +11     
===========================================
+ Hits         34833    34835       +2     
- Misses       14820    14854      +34     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

sonarcloud bot commented Sep 16, 2024

@georgewrmarshall georgewrmarshall requested a review from a team September 18, 2024 17:53
@metamaskbot
Copy link
Collaborator

Builds ready [4aa198b]
Page Load Metrics (2045 ± 79 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint26024621957422203
domContentLoaded17562448201217182
load18052454204516479
domInteractive16435618742
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -146 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

Copy link

sonarcloud bot commented Sep 24, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@metamaskbot
Copy link
Collaborator

Builds ready [6bc33be]
Page Load Metrics (1919 ± 132 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint156925911926275132
domContentLoaded152225761902273131
load155525951919274132
domInteractive14167483617
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -146 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

@georgewrmarshall
Copy link
Contributor

OK to merge without SonarCloud Code Analysis

@georgewrmarshall georgewrmarshall merged commit af8fa34 into develop Sep 25, 2024
76 of 77 checks passed
@georgewrmarshall georgewrmarshall deleted the devin/typography-xyz branch September 25, 2024 16:36
@github-actions github-actions bot locked and limited conversation to collaborators Sep 25, 2024
@metamaskbot metamaskbot added release-12.6.0 Issue or pull request that will be included in release 12.6.0 release-12.5.0 Issue or pull request that will be included in release 12.5.0 and removed release-12.6.0 Issue or pull request that will be included in release 12.6.0 labels Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-12.5.0 Issue or pull request that will be included in release 12.5.0 team-ai AI team (for the Devin AI bot) team-design-system All issues relating to design system in Extension
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Replace deprecated Typography with Text component
4 participants