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

fix: center token icon #26013

Merged
merged 8 commits into from
Jul 23, 2024
Merged

Conversation

sahar-fehri
Copy link
Contributor

@sahar-fehri sahar-fehri commented Jul 22, 2024

Description

This PR centers the tokenIcon in MultichainTokenListItem

Open in GitHub Codespaces

Related issues

Fixes:
https://consensyssoftware.atlassian.net/browse/MMASSETS-251

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

Screenshot 2024-07-22 at 18 18 16 Screenshot 2024-07-22 at 18 18 29

Asset picker modal

Screenshot 2024-07-22 at 18 30 48

After

Screenshot 2024-07-22 at 18 17 22 Screenshot 2024-07-22 at 18 17 47

Asset Picker modal

Screenshot 2024-07-22 at 18 28 47

Pre-merge author checklist

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.

@sahar-fehri sahar-fehri requested a review from a team as a code owner July 22, 2024 16:16
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.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

This PR centers the token icon in the MultichainTokenListItem by adjusting the CSS properties.

  • Modified ui/components/component-library/badge-wrapper/badge-wrapper.scss to change .mm-badge-wrapper's align-self from start to center.
  • Ensure this change does not negatively impact other components relying on the previous alignment.

1 file(s) reviewed, 1 comment(s)
Edit PR Review Bot Settings

@@ -36,7 +36,7 @@
--badge-wrapper-position-rectangular: 0;

position: relative;
align-self: start; // prevents stretching of badge-wrapper when in flexbox container to maintain badge positioning
align-self: center; // prevents stretching of badge-wrapper when in flexbox container to maintain badge positioning
Copy link

Choose a reason for hiding this comment

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

Info: Changed align-self from start to center to center the badge wrapper in its flexbox container.

@sahar-fehri sahar-fehri requested a review from a team as a code owner July 22, 2024 16:27
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

This PR centers the token icon in the MultichainTokenListItem by adjusting CSS properties and adding a class to the BadgeWrapper component.

  • Modified ui/components/component-library/badge-wrapper/badge-wrapper.scss to change .mm-badge-wrapper's align-self from center to start.
  • Updated ui/components/multichain/token-list-item/index.scss to add a new CSS rule for centering the badge within the token list item.
  • Added className="multichain-token-list-item__badge" to the BadgeWrapper in ui/components/multichain/token-list-item/token-list-item.js.

Ensure these changes do not negatively impact other components relying on the previous alignment.

3 file(s) reviewed, 2 comment(s)
Edit PR Review Bot Settings

Comment on lines +11 to +13
&__badge {
align-self: center;
}
Copy link

Choose a reason for hiding this comment

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

Info: Added CSS rule to center the badge within the token list item.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding custom CSS is the right move here because the Box component doesn't currently support and alignSelf style utility prop

@@ -220,6 +220,7 @@ export const TokenListItem = ({
/>
}
marginRight={3}
className="multichain-token-list-item__badge"
Copy link

Choose a reason for hiding this comment

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

Info: Added className attribute to BadgeWrapper for centering the token icon.

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The PR centers the token icon in the MultichainTokenListItem by adjusting CSS properties and adding a class to the BadgeWrapper component.

  • Modified ui/components/component-library/badge-wrapper/badge-wrapper.scss to change .mm-badge-wrapper's align-self from center to start.
  • Updated ui/components/multichain/token-list-item/index.scss to add a new CSS rule for centering the badge within the token list item.
  • Added className="multichain-token-list-item__badge" to the BadgeWrapper in ui/components/multichain/token-list-item/token-list-item.js.

Ensure these changes do not negatively impact other components relying on the previous alignment.

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The PR centers the token icon in the MultichainTokenListItem by adjusting CSS properties and adding a class to the BadgeWrapper component.

  • Modified ui/components/component-library/badge-wrapper/badge-wrapper.scss to change .mm-badge-wrapper's align-self from center to start.
  • Updated ui/components/multichain/token-list-item/index.scss to add a new CSS rule for centering the badge within the token list item.
  • Added className="multichain-token-list-item__badge" to the BadgeWrapper in ui/components/multichain/token-list-item/token-list-item.js.

Ensure these changes do not negatively impact other components relying on the previous alignment.

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The PR centers the token icon in the MultichainTokenListItem by adjusting CSS properties and adding a class to the BadgeWrapper component.

  • Modified ui/components/component-library/badge-wrapper/badge-wrapper.scss to change .mm-badge-wrapper's align-self from center to start.
  • Updated ui/components/multichain/token-list-item/index.scss to add a new CSS rule for centering the badge within the token list item.
  • Added className="multichain-token-list-item__badge" to the BadgeWrapper in ui/components/multichain/token-list-item/token-list-item.js.

Ensure these changes do not negatively impact other components relying on the previous alignment.

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The PR centers the token icon in the MultichainTokenListItem by adjusting CSS properties and adding a class to the BadgeWrapper component.

  • Modified ui/components/component-library/badge-wrapper/badge-wrapper.scss to change .mm-badge-wrapper's align-self from center to start.
  • Updated ui/components/multichain/token-list-item/index.scss to add a new CSS rule for centering the badge within the token list item.
  • Added className="multichain-token-list-item__badge" to the BadgeWrapper in ui/components/multichain/token-list-item/token-list-item.js.

Ensure these changes do not negatively impact other components relying on the previous alignment.

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

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! It might also be worth sharing this with the @MetaMask/wallet-ux team

  • Checked update alignment of AvatarToken in MultichainTokenListItem in storybook ✅

Comment on lines +11 to +13
&__badge {
align-self: center;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding custom CSS is the right move here because the Box component doesn't currently support and alignSelf style utility prop

@sahar-fehri sahar-fehri added the needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. label Jul 23, 2024
@metamaskbot
Copy link
Collaborator

Builds ready [88d04bb]
Page Load Metrics (251 ± 261 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint711711072311
domContentLoaded107931168
load412201251543261
domInteractive107831168
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 46 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The PR centers the token icon in the MultichainTokenListItem by adjusting CSS properties and adding a class to the BadgeWrapper component.

  • Modified ui/components/component-library/badge-wrapper/badge-wrapper.scss to change .mm-badge-wrapper's align-self from center to start.
  • Updated ui/components/multichain/token-list-item/index.scss to add a new CSS rule for centering the badge within the token list item.
  • Added className="multichain-token-list-item__badge" to the BadgeWrapper in ui/components/multichain/token-list-item/token-list-item.js.

Ensure these changes do not negatively impact other components relying on the previous alignment.

51 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.69%. Comparing base (5272c59) to head (1a55669).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #26013   +/-   ##
========================================
  Coverage    69.69%   69.69%           
========================================
  Files         1405     1405           
  Lines        49723    49723           
  Branches     13740    13740           
========================================
  Hits         34650    34650           
  Misses       15073    15073           

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

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

The PR centers the token icon in the MultichainTokenListItem by adjusting CSS properties and adding a class to the BadgeWrapper component.

  • Modified .circleci/config.yml to remove the test-e2e-swap-playwright - OPTIONAL job, potentially streamlining the CI/CD pipeline.
  • Updated app/scripts/background.js to introduce FakeLedgerBridge and FakeTrezorBridge for testing.
  • Added FakeKeyringBridge type in app/scripts/lib/hardware-keyring-builder-factory.ts for mock bridge implementation.
  • Enhanced ui/components/multichain/token-list-item/token-list-item.js to center the token icon by modifying CSS and adding a class.
  • Introduced test/stub/keyring-bridge.js to simulate keyring bridges for testing purposes.

28 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@metamaskbot
Copy link
Collaborator

Builds ready [1a55669]
Page Load Metrics (79 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint673051085024
domContentLoaded10104302211
load40291795225
domInteractive10104302211
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 46 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@sahar-fehri sahar-fehri merged commit 40f61bc into develop Jul 23, 2024
76 checks passed
@sahar-fehri sahar-fehri deleted the fix/fix-css-multichain-token-list-item branch July 23, 2024 17:34
@github-actions github-actions bot locked and limited conversation to collaborators Jul 23, 2024
@metamaskbot metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs-assets-ux-review A shared label between the Assets and UX team to flag PRs ready for consolidated team review. release-12.3.0 Issue or pull request that will be included in release 12.3.0 team-assets
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants