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: remove halo for tokens #26016

Merged
merged 9 commits into from
Jul 24, 2024

Conversation

sahar-fehri
Copy link
Contributor

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

Description

In the Extension, we display a halo around the token avatar, which differs from Mobile and Portfolio. This smaller avatar size makes the avatar less legible and creates an unpolished feel of the product.

This PR removes the halo for the main token list component, the import token modal and the asset picker

Open in GitHub Codespaces

Related issues

Fixes:

Manual testing steps

  1. Go to this page...

Screenshots/Recordings

Before

Screenshot 2024-07-22 at 18 47 15 Screenshot 2024-07-22 at 18 46 56 Screenshot 2024-07-22 at 18 46 45 Screenshot 2024-07-22 at 18 47 02

After

Screenshot 2024-07-22 at 18 42 31 Screenshot 2024-07-22 at 18 42 45 Screenshot 2024-07-22 at 18 42 55 Screenshot 2024-07-22 at 18 48 53

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:45
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 removes the halo effect around token avatars to improve legibility and visual consistency across different components.

  • ui/components/app/import-token/token-list/token-list.component.js: Removed showHalo prop from AvatarToken.
  • ui/components/multichain/asset-picker-amount/asset-picker/asset-picker.tsx: Deleted showHalo prop from AvatarToken.
  • ui/components/multichain/token-list-item/token-list-item.js: Eliminated showHalo prop from AvatarToken.

These changes ensure a more polished and consistent user experience.

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

@@ -108,7 +108,6 @@ export default class TokenList extends Component {
<AvatarToken
Copy link

Choose a reason for hiding this comment

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

Info: Removed the showHalo prop from AvatarToken to eliminate the halo effect around token avatars.

Comment on lines 199 to 203
borderRadius={isNFT ? BorderRadius.LG : BorderRadius.full}
src={primaryTokenImage}
size={AvatarTokenSize.Md}
showHalo={!isNFT}
name={symbol}
{...(isNFT && { backgroundColor: BackgroundColor.transparent })}
Copy link

Choose a reason for hiding this comment

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

Info: Removed showHalo prop from AvatarToken to eliminate the halo effect.

@@ -224,7 +224,6 @@ export const TokenListItem = ({
<AvatarToken
Copy link

Choose a reason for hiding this comment

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

Info: Removed showHalo prop from AvatarToken to improve visual consistency.

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)

No major changes found since last review.

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)

This PR removes the halo effect from token avatars in the main token list, import token modal, and asset picker for a more polished look.

  • Removed halo-related CSS classes from /ui/components/component-library/avatar-token/avatar-token.scss.
  • Updated AvatarToken component in /ui/components/component-library/avatar-token/avatar-token.tsx to exclude halo logic.
  • Modified ImportToken component in /ui/pages/swaps/import-token/import-token.js to reflect the updated AvatarToken component without the halo.

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)

This PR removes the halo effect from token avatars in the main token list, import token modal, and asset picker for a more polished look.

  • Removed halo-related CSS classes from /ui/components/component-library/avatar-token/avatar-token.scss.
  • Updated AvatarToken component in /ui/components/component-library/avatar-token/avatar-token.tsx to exclude halo logic.
  • Modified ImportToken component in /ui/pages/swaps/import-token/import-token.js to reflect the updated AvatarToken component without the halo.
  • Adjusted ImportTokensModalConfirm in /ui/components/multichain/import-tokens-modal/import-tokens-modal-confirm.js to remove halo from token avatars.
  • Updated TokenDetailsPage in /ui/pages/token-details/token-details-page.js to ensure consistency in token avatar display.

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)

This PR removes the halo effect from token avatars in the main token list, import token modal, and asset picker for a more polished look.

  • Removed halo-related CSS classes from /ui/components/component-library/avatar-token/avatar-token.scss.
  • Updated AvatarToken component in /ui/components/component-library/avatar-token/avatar-token.tsx to exclude halo logic.
  • Modified ImportToken component in /ui/pages/swaps/import-token/import-token.js to reflect the updated AvatarToken component without the halo.
  • Adjusted ImportTokensModalConfirm in /ui/components/multichain/import-tokens-modal/import-tokens-modal-confirm.js to remove halo from token avatars.
  • Updated TokenDetailsPage in /ui/pages/token-details/token-details-page.js to ensure consistency in token avatar display.

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)

This PR removes the halo effect from token avatars in the main token list, import token modal, and asset picker for a more polished look.

  • Removed halo-related CSS classes from /ui/components/component-library/avatar-token/avatar-token.scss.
  • Updated AvatarToken component in /ui/components/component-library/avatar-token/avatar-token.tsx to exclude halo logic.
  • Modified ImportToken component in /ui/pages/swaps/import-token/import-token.js to reflect the updated AvatarToken component without the halo.
  • Adjusted ImportTokensModalConfirm in /ui/components/multichain/import-tokens-modal/import-tokens-modal-confirm.js to remove halo from token avatars.
  • Updated TokenDetailsPage in /ui/pages/token-details/token-details-page.js to ensure consistency in token avatar display.

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)

This PR removes the halo effect from token avatars in the main token list, import token modal, and asset picker for a more polished look.

  • Removed halo-related CSS classes from /ui/components/component-library/avatar-token/avatar-token.scss.
  • Updated AvatarToken component in /ui/components/component-library/avatar-token/avatar-token.tsx to exclude halo logic.
  • Modified ImportToken component in /ui/pages/swaps/import-token/import-token.js to reflect the updated AvatarToken component without the halo.
  • Adjusted ImportTokensModalConfirm in /ui/components/multichain/import-tokens-modal/import-tokens-modal-confirm.js to remove halo from token avatars.
  • Updated TokenDetailsPage in /ui/pages/token-details/token-details-page.js to ensure consistency in token avatar display.

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.

Looking good! I did a search for showHalo and found one instance that hasn't been removed. I'm guessing import-tokens-modal-confirm should be updated too?

<AvatarToken name={symbol} src={tokenImage} showHalo />

Screenshot 2024-07-22 at 2 46 49 PM

Checked showHalo removed for following components in storybook

  • ui/components/app/import-token/token-list/token-list.component.js
  • ui/components/multichain/asset-picker-amount/asset-picker-amount.tsx for AssetPicker component ✅
  • ui/components/multichain/token-list-item/token-list-item.js

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 removes the halo effect from token avatars in the main token list, import token modal, and asset picker for a more polished look.

  • Removed showHalo prop from AvatarToken in ui/components/multichain/import-tokens-modal/import-tokens-modal-confirm.js.
  • Updated AvatarToken component in ui/components/component-library/avatar-token/avatar-token.tsx to exclude halo logic.
  • Removed halo-related CSS classes from ui/components/component-library/avatar-token/avatar-token.scss.
  • Modified ImportToken component in ui/pages/swaps/import-token/import-token.js to reflect the updated AvatarToken component without the halo.

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

Copy link

@metamaskbot
Copy link
Collaborator

Builds ready [427efd3]
Page Load Metrics (142 ± 148 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint712621023919
domContentLoaded96928147
load391477142308148
domInteractive96928147
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -48 Bytes (-0.00%)
  • common: 0 Bytes (0.00%)

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 (427efd3).
Report is 6 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #26016   +/-   ##
========================================
  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)

This PR removes the halo effect from token avatars in the main token list, import token modal, and asset picker for a more polished look.

  • Removed showHalo prop from AvatarToken in ui/components/multichain/import-tokens-modal/import-tokens-modal-confirm.js.
  • Updated AvatarToken component in ui/components/component-library/avatar-token/avatar-token.tsx to exclude halo logic.
  • Removed halo-related CSS classes from ui/components/component-library/avatar-token/avatar-token.scss.
  • Modified ImportToken component in ui/pages/swaps/import-token/import-token.js to reflect the updated AvatarToken component without the halo.
  • Updated TokenListItem in ui/components/multichain/token-list-item/token-list-item.js to remove the halo effect from token avatars.

77 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!

@sahar-fehri sahar-fehri merged commit 3d2a436 into develop Jul 24, 2024
76 checks passed
@sahar-fehri sahar-fehri deleted the fix/fix-css-remove-halo-from-token-icon branch July 24, 2024 17:58
@github-actions github-actions bot locked and limited conversation to collaborators Jul 24, 2024
@metamaskbot metamaskbot added the release-12.3.0 Issue or pull request that will be included in release 12.3.0 label Jul 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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