Add color indicator component #10209

brad-decker · 2021-01-18T22:05:35Z

Depends on: #10213

Adds a color indicator component that will replace all instances of circle colors in the app. A future pull request will use the color indicator and replace:

Network indicator color in the app bar
Network indicator in network dropdown
Network indicator in confirmation pages
Network indicator on the network settings page.
Connected accounts indicator

metamaskbot · 2021-01-19T18:34:54Z

Builds ready [a90b44c]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (490 ± 30 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	30	54	44	8	4
		domContentLoaded	330	540	488	63	30
		load	332	541	490	63	30
		domInteractive	330	539	488	63	30

metamaskbot · 2021-01-20T16:16:41Z

Builds ready [a3528a1]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (602 ± 59 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	35	93	56	14	7
		domContentLoaded	358	890	601	123	59
		load	360	891	602	123	59
		domInteractive	358	890	600	123	59

.storybook/main.js

metamaskbot · 2021-01-20T22:39:00Z

Builds ready [58e4c35]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (559 ± 44 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	35	67	51	8	4
		domContentLoaded	331	827	557	92	44
		load	333	828	559	92	44
		domInteractive	331	827	557	92	44

metamaskbot · 2021-01-21T17:30:04Z

Builds ready [2a7db8d]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (643 ± 65 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	51	87	64	10	5
		domContentLoaded	378	863	642	136	65
		load	379	865	643	136	65
		domInteractive	377	863	641	136	65

metamaskbot · 2021-01-21T18:06:20Z

Builds ready [1a63ff0]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (511 ± 36 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	40	62	50	7	3
		domContentLoaded	338	574	509	74	36
		load	340	575	511	74	36
		domInteractive	338	573	509	74	36

Gudahtt · 2021-01-22T04:39:36Z

ui/app/components/ui/color-indicator/color-indicator.scss

+    }
+  }
+
+  // separate iterator to ensure borderColor takes precedence


When testing this locally in storybook, it seems that this border-color never actually has precedence. I'm just seeing the color-indicator--color-{variant} border color 🤔

Actually, no, it's only broken when I set the border colour to one of the ERROR colours.

Oh I see - there's a mistake in the design system constants. I can fix that in a separate PR.

Gudahtt · 2021-01-22T04:51:18Z

ui/app/components/ui/color-indicator/color-indicator.js

+ColorIndicator.propTypes = {
+  color: PropTypes.oneOf(Object.values(COLORS)),
+  borderColor: PropTypes.oneOf(Object.values(COLORS)),
+  size: PropTypes.oneOf(['small', 'medium', 'large']),


Nit: It'd be nice to use constants for these as well, and for the type PropType declaration.

ui/app/css/utilities/_colors.scss

Gudahtt · 2021-01-22T05:00:04Z

ui/app/css/utilities/_colors.scss

+ * Calculate the luminance for a color.
+ * See https://www.w3.org/TR/WCAG20-TECHS/G17.html#G17-tests
+ */
+@function luminance($color) {


Wow, this is a pretty complex function!

I did some Google-ing about this and stumbled across this interesting post: https://css-tricks.com/programming-sass-to-create-accessible-color-combinations/
They reported that when using this method:

Unfortunately, calculating only a handful of color contrast combinations increased Sass build times exponentially

But they managed to solve it more performantly in the end using a lookup table.

Not saying we should optimize this now before it becomes a problem, but... this is something to consider if we notice a slowdown.

i'm going to leave a note in the code about this. I think that the expensive part of the operation was the custom pow function that didn't exist in older versions of sass. To get around this some implementations implemented functions that do many loops. Since then, math.pow is a language level feature (https://github.com/sass/dart-sass/releases/tag/1.25.0). I tried to find any mentions of it's performance, but came up empty.

Ah neat, TIL.

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

brad-decker · 2021-01-22T16:14:45Z

Ready for you again, @Gudahtt! Thanks for the code review.

Gudahtt

LGTM! I'm excited to have that choose-contrast-color function available now, that's pretty rad

metamaskbot · 2021-01-22T16:31:06Z

Builds ready [f70f9e4]

builds: chrome, firefox, opera
bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
code coverage: Report
dep viz: background
all artifacts

Page Load Metrics (572 ± 37 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	45	66	56	6	3
		domContentLoaded	355	650	571	77	37
		load	356	651	572	77	37
		domInteractive	354	650	570	77	37

brad-decker changed the base branch from develop to add-chip-component January 18, 2021 22:05

brad-decker changed the title ~~improve design system scss~~ Add color indicator component Jan 19, 2021

brad-decker force-pushed the add-color-indicator branch 2 times, most recently from 578f838 to ba48b77 Compare January 19, 2021 16:23

brad-decker force-pushed the add-chip-component branch 2 times, most recently from 7b859b5 to 3baa8dd Compare January 19, 2021 18:06

brad-decker mentioned this pull request Jan 19, 2021

add includePaths to sass-loader in storybook #10213

Merged

brad-decker force-pushed the add-chip-component branch from 3baa8dd to 0fa8434 Compare January 19, 2021 18:15

brad-decker force-pushed the add-color-indicator branch from ba48b77 to a90b44c Compare January 19, 2021 18:17

brad-decker changed the base branch from add-chip-component to add-scss-path January 19, 2021 18:18

brad-decker mentioned this pull request Jan 19, 2021

Refactor NetworkDisplay, ConnectedStatusIndicator, etc to use ColorIndicator #10214

Merged

Base automatically changed from add-scss-path to develop January 20, 2021 15:58

brad-decker force-pushed the add-color-indicator branch from a90b44c to a3528a1 Compare January 20, 2021 16:00

brad-decker marked this pull request as ready for review January 20, 2021 16:00

brad-decker requested a review from a team as a code owner January 20, 2021 16:00

brad-decker requested a review from rekmarks January 20, 2021 16:00

brad-decker commented Jan 20, 2021

View reviewed changes

.storybook/main.js Outdated Show resolved Hide resolved

brad-decker force-pushed the add-color-indicator branch from a3528a1 to 58e4c35 Compare January 20, 2021 22:20

add ColorIndicator component

2a7db8d

brad-decker force-pushed the add-color-indicator branch from 58e4c35 to 2a7db8d Compare January 21, 2021 17:13

fix relative urls in css

1a63ff0

Gudahtt reviewed Jan 22, 2021

View reviewed changes

brad-decker and others added 2 commits January 22, 2021 10:06

Update ui/app/css/utilities/_colors.scss

872caac

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

use constants

d4a2f20

notate possible perf issue

f70f9e4

Gudahtt approved these changes Jan 22, 2021

View reviewed changes

brad-decker merged commit a036b0e into develop Jan 22, 2021

brad-decker deleted the add-color-indicator branch January 22, 2021 21:45

github-actions bot locked and limited conversation to collaborators Jan 22, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add color indicator component #10209

Add color indicator component #10209

brad-decker commented Jan 18, 2021 •

edited

Loading

metamaskbot commented Jan 19, 2021

metamaskbot commented Jan 20, 2021

metamaskbot commented Jan 20, 2021

metamaskbot commented Jan 21, 2021

metamaskbot commented Jan 21, 2021

Gudahtt Jan 22, 2021

Gudahtt Jan 22, 2021

Gudahtt Jan 22, 2021

Gudahtt Jan 22, 2021

Gudahtt Jan 22, 2021

Gudahtt Jan 22, 2021

brad-decker Jan 22, 2021

Gudahtt Jan 22, 2021

brad-decker commented Jan 22, 2021

Gudahtt left a comment

metamaskbot commented Jan 22, 2021

Add color indicator component #10209

Add color indicator component #10209

Conversation

brad-decker commented Jan 18, 2021 • edited Loading

metamaskbot commented Jan 19, 2021

metamaskbot commented Jan 20, 2021

metamaskbot commented Jan 20, 2021

metamaskbot commented Jan 21, 2021

metamaskbot commented Jan 21, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brad-decker commented Jan 22, 2021

Gudahtt left a comment

Choose a reason for hiding this comment

metamaskbot commented Jan 22, 2021

brad-decker commented Jan 18, 2021 •

edited

Loading