colorMode key improvements; computed dependencies update
#44
Conversation
|
^ is happening in the target branch, so this does not need to be solved in this branch. It will likely be difficult to resolve before there is a clearer theme shape, and themes (default, ams) match. |
| light: { | ||
| success: computed(['colors.primary'], () => '#85E89d'), | ||
| success: computed(() => '#85E89d', ['colors.primary']), |
There was a problem hiding this comment.
Just double-checking that this doesn't actually need to be computed because it seems to just be returning a straight hex value?
| success: computed(() => '#85E89d', ['colors.primary']), | |
| success: '#85E89d', |
There was a problem hiding this comment.
Nope. Just left over from some check I was doing.
| thin: computed(([widthThin, color]) => `${widthThin} solid ${color}`, [ | ||
| 'border.widthThin', | ||
| 'border.color', | ||
| ]), |
There was a problem hiding this comment.
Is this a possible different way to write this, do does the array values have to be a single key?
| thin: computed(([widthThin, color]) => `${widthThin} solid ${color}`, [ | |
| 'border.widthThin', | |
| 'border.color', | |
| ]), | |
| thin: computed(([border]) => `${border.widthThin} solid ${border.color}`, ['border']), |
There was a problem hiding this comment.
Also could we simplify further to allow the second param to be a single value as well as an array like:
| thin: computed(([widthThin, color]) => `${widthThin} solid ${color}`, [ | |
| 'border.widthThin', | |
| 'border.color', | |
| ]), | |
| thin: computed((border) => `${border.widthThin} solid ${border.color}`, 'border'), |
There was a problem hiding this comment.
Is this a possible different way to write this, do does the array values have to be a single key?
This is similar to object spreading, and I haven't been able to make it work yet. It's on my radar, though, and I'll keep exploring options.
Also could we simplify further to allow the second param to be a single value as well as an array like
Sure, we could allow single dependency values in addition to arrays.
| textPrimary: computed((theme) => makeHighContrastColor(theme.colors.primary)), | ||
| textAccent: computed(([accent]) => makeHighContrastColor(accent), [ | ||
| 'colors.accent', | ||
| ]), |
There was a problem hiding this comment.
This might have answered my question above. Just want to know what the preferred guidance is for these computed values. Is one method better than the other?
There was a problem hiding this comment.
It's entirely up to consumer preference. I'm not sure there's a large enough computational difference to say that one is better than the other.
| xxl: computed([`${COLOR_MODE_KEY}.base`], ([base]) => `${base * 2.5}px`), | ||
| xs: computed(sizeToPixel(0.25)), |
There was a problem hiding this comment.
This is fantastic! So much simpler! Is this then even something a consumer could do?
There was a problem hiding this comment.
Yep, consumers can use utility functions like this if we export them, or they can write their own curried/partial functions to do similar things.
| @@ -69,7 +69,7 @@ const font: EuiFont = { | |||
|
|
|||
| // Font weights | |||
| const fontWeight = { | |||
| light: '300', | |||
| weightLight: '300', // TODO | |||
There was a problem hiding this comment.
Yeah so we talked about maybe for the color mode keys, they're uppercased like LIGHT and DARK leaving any lowercase versions allowed as normal keys here. But I'm fine with this for now.
There was a problem hiding this comment.
I'll change this back before merging. I think it's certain we won't use light and dark as color mode keys, so moving to something else right now makes sense.
|
Gonna make a couple changes before merging this |
|
All done. Apparently only you can merge PRs here |
Summary
Again, more for discussion than to merge this in its current state. Let's see if the changes are closer to your ideal API