Conversation
578f838 to
ba48b77
Compare
7b859b5 to
3baa8dd
Compare
3baa8dd to
0fa8434
Compare
ba48b77 to
a90b44c
Compare
Builds ready [a90b44c]
Page Load Metrics (490 ± 30 ms)
|
a90b44c to
a3528a1
Compare
Builds ready [a3528a1]
Page Load Metrics (602 ± 59 ms)
|
a3528a1 to
58e4c35
Compare
Builds ready [58e4c35]
Page Load Metrics (559 ± 44 ms)
|
58e4c35 to
2a7db8d
Compare
Builds ready [2a7db8d]
Page Load Metrics (643 ± 65 ms)
|
Builds ready [1a63ff0]
Page Load Metrics (511 ± 36 ms)
|
| } | ||
| } | ||
|
|
||
| // separate iterator to ensure borderColor takes precedence |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
Actually, no, it's only broken when I set the border colour to one of the ERROR colours.
There was a problem hiding this comment.
Oh I see - there's a mistake in the design system constants. I can fix that in a separate PR.
| ColorIndicator.propTypes = { | ||
| color: PropTypes.oneOf(Object.values(COLORS)), | ||
| borderColor: PropTypes.oneOf(Object.values(COLORS)), | ||
| size: PropTypes.oneOf(['small', 'medium', 'large']), |
There was a problem hiding this comment.
Nit: It'd be nice to use constants for these as well, and for the type PropType declaration.
| * Calculate the luminance for a color. | ||
| * See https://www.w3.org/TR/WCAG20-TECHS/G17.html#G17-tests | ||
| */ | ||
| @function luminance($color) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
|
Ready for you again, @Gudahtt! Thanks for the code review. |
Gudahtt
left a comment
There was a problem hiding this comment.
LGTM! I'm excited to have that choose-contrast-color function available now, that's pretty rad
Builds ready [f70f9e4]
Page Load Metrics (572 ± 37 ms)
|
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: