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

EUI should export hex values instead of rgb() values #1588

Closed
sorenlouv opened this issue Feb 22, 2019 · 9 comments
Closed

EUI should export hex values instead of rgb() values #1588

sorenlouv opened this issue Feb 22, 2019 · 9 comments

Comments

@sorenlouv
Copy link
Member

The json file that is used by consumers who use css-in-js exports colors as rgb. This is different from the colors exported by sass which is hex.

Not all EUI components accept rgb values css-in-js consumers have to convert the rgb colors to hex. This is an additional step and it would make more sense if the exported colors were hex in the first place.

Notes to aid in implementing the change

Script that exports SCSS variables as json
https://github.com/elastic/eui/blob/master/scripts/compile-scss.js

How to run the export script (in EUI repo)
node scripts/compile-scss.js

Colors are converted to rgb in the sass-extract-js plugin
We can fork this plugin, and change it to export hex
https://github.com/adamgruber/sass-extract-js/blob/master/lib/transformVars.js#L37-L45

Sasscolor format (used in sass-extract-js)
https://github.com/jgranstrom/sass-extract#sasscolor

@sorenlouv
Copy link
Member Author

@weltenwort @chandlerprall I've forked sass-extract-js and made the necessary change: sorenlouv/sass-extract-js@23a7958.

For EUI to use this, I need to publish a new npm package. Should I publish it under my own name, or @elastic?

@weltenwort
Copy link
Member

Given that the plugin is essentially one file, have you considered just including a customized version directly alongside the export script?

@chandlerprall
Copy link
Contributor

I'd expect this to break existing code processing those values, have you tested the generated json in Kibana?

Agreed with @weltenwort about placement/inclusion - though it will need a note retaining the license. We could drop the whole thing in eui's packages next to our internal react-datepicker fork.

@snide
Copy link
Contributor

snide commented Feb 22, 2019

I know this is probably gonna be something we don't want to do, but another problem with the exports is that they reformat the casing of the variables in the Sass. This makes our documentation inconsistent. euiSizeXS becomes euiSizeXs. This was an oversight on my part in review when Felix added this stuff because it's supported from the lib.

Is there any interest in doing a one time variable cleanup across Kibana to get us back on track if we changed this setting? Pinging @elastic/cloud-ui as well since I know @bevacqua got confused by it once before as well.

@weltenwort
Copy link
Member

weltenwort commented Feb 22, 2019

I'd expect this to break existing code processing those values, have you tested the generated json in Kibana?

As far as the Infra UI is concerned we mostly either including the values in styles or passing it to functions that can parse most color formats. So I wouldn't expect any breakage there.

Is there any interest in doing a one time variable cleanup across Kibana to get us back on track if we changed this setting?

I wouldn't mind contributing any Infra UI fixes to the PR that performs the backwards-imcompatible EUI upgrade in Kibana. Just let me know.

@sorenlouv
Copy link
Member Author

I'd expect this to break existing code processing those values, have you tested the generated json in Kibana?

I haven't tested yet but will :)
Before starting this I did a quick search, and only consumers of the json file was Infra (aka @weltenwort), whom I already talked to.

@sorenlouv
Copy link
Member Author

sorenlouv commented Feb 25, 2019

Given that the plugin is essentially one file, have you considered just including a customized version directly alongside the export script?

I'm fine with that!

though it will need a note retaining the license.

Not strong in licenses so might need a bit of help here.

@sorenlouv
Copy link
Member Author

sorenlouv commented Feb 25, 2019

another problem with the exports is that they reformat the casing of the variables in the Sass. This makes our documentation inconsistent. euiSizeXS becomes euiSizeXs

I removed the camelCasing but it introduces some new problems: fontSize becomes font-size, which is bit awkward to access as a js property: theme["font-size"] vs theme.fontSize.

Full diff of the light theme with the new variables:
https://www.diffchecker.com/XyqCx9Cs

@sorenlouv
Copy link
Member Author

Actually, there doesn't seem to be a real issue with removing the camelCasing logic. It is only avatarSizing that is affected in a weird way. Rest is for the better.

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

No branches or pull requests

5 participants