UX Icon: Updated Icon for Disclosure #17877

NidhiKJha · 2023-02-23T11:54:26Z

This PR is to update fa-add with ICON_NAMES.ADD

Before

After

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.

metamaskbot · 2023-02-23T12:16:22Z

Builds ready [a601769]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1733 ± 77 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	100	177	130	20	10
		domContentLoaded	1515	2103	1714	157	75
		load	1515	2103	1733	160	77
		domInteractive	1514	2103	1714	157	75

Bundle size diffs

background: 0 bytes
ui: 332 bytes
common: 0 bytes

darkwing · 2023-02-23T13:26:24Z

ui/components/ui/disclosure/disclosure.scss


-    i {
+    &--icon {


@georgewrmarshall Am I correct in that we want to remove these styles completely and instead add marginInlineStart={x} and marginInlineEnd={x}?

We can add the maginInlineStart={-5} and that should work but I am not sure about using marginInlineEnd since its value is 10 here.

We can just round down to the closest divisible by 4 value so 8px marginInlineEnd={2} I don't think the box component supports negative values though so -5 won't work. Do you know what the reason is for the negative margin? It seems quite random?

Thanks @georgewrmarshall @darkwing . I have updated the marginInlineEnd value. Seems like the negative margin was added to keep the icon in place, since padding-left: 24px; in disclosure-summary was adding extra padding to the container.

New

georgewrmarshall

Checked storybook looks good. Would like to see what the reason is for the -20px margin. Left a comment under the margin thread

georgewrmarshall · 2023-03-01T04:34:42Z

ui/components/ui/disclosure/disclosure.scss


-    i {
+    &--icon {


We can just round down to the closest divisible by 4 value so 8px marginInlineEnd={2} I don't think the box component supports negative values though so -5 won't work. Do you know what the reason is for the negative margin? It seems quite random?

garrettbear · 2023-03-06T20:14:35Z

ui/components/ui/disclosure/disclosure.js

@@ -23,7 +25,12 @@ const Disclosure = ({ children, title, size }) => {
      {title ? (
        <details>
          <summary className="disclosure__summary">
-            <i className="fa fa-plus" />
+            <Icon
+              className="disclosure__summary--icon"


In BEM standards would still keep this as disclosure__summary-icon and reserving the double dash -- for when it is a modifier (e.g. disclosure__summary-icon--size-xs)

Suggested change

className="disclosure__summary--icon"

className="disclosure__summary-icon"

garrettbear

LGTM - just made the one comment but won't let that hold you back from completing task

georgewrmarshall

Code looks good just need a storybook build for visual review

codecov · 2023-03-08T11:46:03Z

Codecov Report

Merging #17877 (c8c8ac7) into develop (601e02c) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop   #17877   +/-   ##
========================================
  Coverage    63.45%   63.45%           
========================================
  Files          903      903           
  Lines        35264    35264           
  Branches      8919     8919           
========================================
  Hits         22374    22374           
  Misses       12890    12890

Impacted Files	Coverage Δ
ui/components/ui/disclosure/disclosure.js	`62.50% <ø> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

metamaskbot · 2023-03-08T11:51:15Z

Builds ready [c8c8ac7]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1602 ± 54 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	95	149	114	15	7
		domContentLoaded	1353	1897	1579	116	56
		load	1451	1898	1602	112	54
		domInteractive	1353	1897	1579	116	56

Bundle size diffs

background: 0 bytes
ui: 350 bytes
common: 0 bytes

georgewrmarshall

LGTM

NidhiKJha added 2 commits February 23, 2023 17:12

updated fa-add

ee2ce06

updated summary icon

3fa89f0

NidhiKJha added team-design-system All issues relating to design system in Extension team-extension-client team-extension-ux DEPRECATED: please use "team-wallet-ux" label instead labels Feb 23, 2023

NidhiKJha requested a review from a team as a code owner February 23, 2023 11:54

NidhiKJha requested a review from Gtonizuka February 23, 2023 11:54

darkwing reviewed Feb 23, 2023

View reviewed changes

NidhiKJha requested a review from georgewrmarshall February 23, 2023 13:33

georgewrmarshall reviewed Mar 1, 2023

View reviewed changes

updated styles for disclosure

50cbf8a

NidhiKJha force-pushed the fa-add branch from a601769 to 50cbf8a Compare March 6, 2023 10:07

NidhiKJha and others added 2 commits March 6, 2023 15:40

updated margin value

efd642d

Merge branch 'develop' into fa-add

8bd85a8

NidhiKJha requested review from darkwing and georgewrmarshall March 6, 2023 10:14

NidhiKJha added 2 commits March 6, 2023 19:53

Merge branch 'develop' into fa-add

d7536c2

Merge branch 'develop' into fa-add

408a3d0

garrettbear reviewed Mar 6, 2023

View reviewed changes

garrettbear approved these changes Mar 6, 2023

View reviewed changes

georgewrmarshall reviewed Mar 6, 2023

View reviewed changes

Merge branch 'develop' into fa-add

c8c8ac7

georgewrmarshall approved these changes Mar 8, 2023

View reviewed changes

NidhiKJha merged commit bf06598 into develop Mar 9, 2023

NidhiKJha deleted the fa-add branch March 9, 2023 05:18

github-actions bot locked and limited conversation to collaborators Mar 9, 2023

georgewrmarshall assigned NidhiKJha Jun 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UX Icon: Updated Icon for Disclosure #17877

UX Icon: Updated Icon for Disclosure #17877

NidhiKJha commented Feb 23, 2023

metamaskbot commented Feb 23, 2023

darkwing Feb 23, 2023

NidhiKJha Feb 23, 2023

georgewrmarshall Mar 1, 2023

NidhiKJha Mar 6, 2023

georgewrmarshall left a comment

georgewrmarshall Mar 1, 2023

garrettbear Mar 6, 2023

garrettbear left a comment

georgewrmarshall left a comment

codecov bot commented Mar 8, 2023 •

edited

Loading

metamaskbot commented Mar 8, 2023

georgewrmarshall left a comment

	className="disclosure__summary--icon"
	className="disclosure__summary-icon"

UX Icon: Updated Icon for Disclosure #17877

UX Icon: Updated Icon for Disclosure #17877

Conversation

NidhiKJha commented Feb 23, 2023

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

metamaskbot commented Feb 23, 2023

darkwing Feb 23, 2023

Choose a reason for hiding this comment

NidhiKJha Feb 23, 2023

Choose a reason for hiding this comment

georgewrmarshall Mar 1, 2023

Choose a reason for hiding this comment

NidhiKJha Mar 6, 2023

Choose a reason for hiding this comment

New

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall Mar 1, 2023

Choose a reason for hiding this comment

garrettbear Mar 6, 2023

Choose a reason for hiding this comment

garrettbear left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2023 • edited Loading

Codecov Report

metamaskbot commented Mar 8, 2023

georgewrmarshall left a comment

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2023 •

edited

Loading