Allowed user to use any colour in EuiStat#3617
Conversation
|
Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually? |
chandlerprall
left a comment
There was a problem hiding this comment.
Thanks for the PR! Couple of comments,
- let's prefer
colorToClassNameMap.hasOwnProperty(titleColor)overCOLORS.includes; they are functionally the same, but as the code then looks up the value fromcolorToClassNameMapit is a bit more readable - you don't have to find the relationship betweenCOLORSandcolorToClassNameMapto understand what's happening - The error you saw in the docs is because the type for
titleColoris stillkeyof typeof colorToClassNameMapand needs to be updated tokeyof typeof colorToClassNameMap | string. Keeping the existingkeyof ...part around helps with value auto-completion and our docs, even though it is a bit redundant as those keys are all strings. - updating the type to include
stringwill make the classname lookupcolorToClassNameMap[titleColor]error, so thecolorToClassNameMap.hasOwnProperty(titleColor)logic needs to be extracted to a type guard function,
const isColorClass = (input: string): input is keyof typeof colorToClassNameMap => {
return colorToClassNameMap.hasOwnProperty(input);
};
...
// when building titleClasses
isColorClass(titleColor) ? colorToClassNameMap[titleColor] : null,
|
@chandlerprall Yup, I've made the changes, Should I write some tests too for the hexcode? |
Yes please! I think one should suffice, testing that an arbitrary string (hex code) is passed through the style, and ideally it should verify that none of the color css classes were applied |
|
@chandlerprall I've written the test, could you review it? |
|
jenkins test this |
chandlerprall
left a comment
There was a problem hiding this comment.
Changes LGTM! Pulled and tested locally, will merge on passing CI
|
@chandlerprall I resolved a conflict in the changelog, Do you need to run them again? |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3617/ |
|
I've double checked that only the changelog was modified after last CI run; merging! |
|
Ha! Went a little too quick there, everything showed green but the last run actually failed, looks like a linting error 😓 . I'm gonna clean up my mistake and push directly |
|
@chandlerprall nw, learnt a lot through this one, thanks :) |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3617/ |
Summary
In
EuiStat, User was only allowed to use the default colours, allowed user to enter any hex code for desired colourChecklist
[ ] Checked in mobile[ ] Checked in IE11 and Firefox-~ [ ] Props have proper autodocs~
[ ] Checked Code Sandbox works for the any docs examples[ ] Checked for breaking changes and labeled appropriatelyCloses #3606
@chandlerprall @snide