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

Ux: Icon: Update fa-eye with ICON_NAMES.EYE #17800

Merged
merged 10 commits into from
Feb 28, 2023
Merged

Ux: Icon: Update fa-eye with ICON_NAMES.EYE #17800

merged 10 commits into from
Feb 28, 2023

Conversation

NidhiKJha
Copy link
Member

Explanation

Removes the usage of both the fa-eye with ICON_NAMES.EYE

Screenshots/Screencaps

Before

bef

After

azft

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@digiwand
Copy link
Contributor

Hi @georgewrmarshall,

I'm wondering, where did the new icons come from? If we didn't individually select them by using a bundled set, I wonder if we should consider preserving some old icons and integrating them into the new system

@github-actions
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
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 - once you fix linting issues

@georgewrmarshall
Copy link
Contributor

georgewrmarshall commented Feb 23, 2023

Hey @digiwand, I believe Sara decided on the initial set of icons based on the IA/Nav redesign but we are working to be backwards compatible with what already exists in the extension. Adding the ones we are missing could be a good solution for the time being cc @Akatori-Design https://www.figma.com/file/Ne19tqQetZUNFBPROgu3qm/Icon-Audit?node-id=104%3A109&t=rb33g7TdF27DACsz-1

Katew212
Katew212 previously approved these changes Feb 23, 2023
@digiwand
Copy link
Contributor

@georgewrmarshall I see. Thanks for the info!

digiwand
digiwand previously approved these changes Feb 23, 2023
Copy link
Contributor

@digiwand digiwand left a comment

Choose a reason for hiding this comment

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

Lgtm!

@NidhiKJha NidhiKJha dismissed stale reviews from digiwand and Katew212 via 267703f February 23, 2023 13:49
@NidhiKJha NidhiKJha requested a review from digiwand February 23, 2023 13:50
digiwand
digiwand previously approved these changes Feb 23, 2023
@NidhiKJha NidhiKJha requested a review from digiwand February 23, 2023 21:54
@metamaskbot
Copy link
Collaborator

Builds ready [66bf33d]
Page Load Metrics (1559 ± 36 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint9312911194
domContentLoaded1313170615277837
load1394170615597536
domInteractive1313170615277837
Bundle size diffs
  • background: 0 bytes
  • ui: 519 bytes
  • common: 0 bytes

Comment on lines 18 to +21
///: END:ONLY_INCLUDE_IN
import { Icon, ICON_NAMES } from '../../components/component-library';
import { Color } from '../constants/design-system';
import { coinTypeToProtocolName, getSnapDerivationPathName } from './util'; // eslint-disable-line no-unused-vars
Copy link
Contributor

Choose a reason for hiding this comment

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

[suggestion] I believe we could replace lines 18-21 w/

import { Icon, ICON_NAMES } from '../../components/component-library';
import { Color } from '../constants/design-system';
///: BEGIN:ONLY_INCLUDE_IN(flask)
import { SNAPS_METADATA } from '../../../shared/constants/snaps';
import { coinTypeToProtocolName, getSnapDerivationPathName } from './util';
///: END:ONLY_INCLUDE_IN

to avoid // eslint-disable-line no-unused-vars

@NidhiKJha NidhiKJha merged commit 878bb4a into develop Feb 28, 2023
@NidhiKJha NidhiKJha deleted the fix-fa-e branch February 28, 2023 07:26
@github-actions github-actions bot locked and limited conversation to collaborators Feb 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants