Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowed user to use any colour in EuiStat #3617

Merged
merged 6 commits into from
Jun 16, 2020
Merged

Conversation

shrey
Copy link
Contributor

@shrey shrey commented Jun 16, 2020

Summary

In EuiStat, User was only allowed to use the default colours, allowed user to enter any hex code for desired colour

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • [ ] Checked in mobile
  • [ ] Checked in IE11 and Firefox
    -~ [ ] Props have proper autodocs~
  • Added documentation
  • [ ] Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • [ ] Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Closes #3606
@chandlerprall @snide

@kibanamachine
Copy link

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 chandlerprall self-requested a review June 16, 2020 15:38
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! Couple of comments,

  • let's prefer colorToClassNameMap.hasOwnProperty(titleColor) over COLORS.includes; they are functionally the same, but as the code then looks up the value from colorToClassNameMap it is a bit more readable - you don't have to find the relationship between COLORS and colorToClassNameMap to understand what's happening
  • The error you saw in the docs is because the type for titleColor is still keyof typeof colorToClassNameMap and needs to be updated to keyof typeof colorToClassNameMap | string. Keeping the existing keyof ... 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 string will make the classname lookup colorToClassNameMap[titleColor] error, so the colorToClassNameMap.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,

@shrey
Copy link
Contributor Author

shrey commented Jun 16, 2020

@chandlerprall Yup, I've made the changes, Should I write some tests too for the hexcode?

@chandlerprall
Copy link
Contributor

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

@shrey
Copy link
Contributor Author

shrey commented Jun 16, 2020

@chandlerprall I've written the test, could you review it?

@chandlerprall
Copy link
Contributor

jenkins test this

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM! Pulled and tested locally, will merge on passing CI

@shrey
Copy link
Contributor Author

shrey commented Jun 16, 2020

@chandlerprall I resolved a conflict in the changelog, Do you need to run them again?

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3617/

@chandlerprall
Copy link
Contributor

I've double checked that only the changelog was modified after last CI run; merging! :shipit:

@chandlerprall chandlerprall merged commit ee87dd0 into elastic:master Jun 16, 2020
@chandlerprall
Copy link
Contributor

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

@shrey
Copy link
Contributor Author

shrey commented Jun 16, 2020

@chandlerprall nw, learnt a lot through this one, thanks :)

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_3617/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiStat] Allow for customization of colors
3 participants