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

[CSS-in-JS] Sizes with units #4578

Merged
merged 8 commits into from
Mar 15, 2021

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Feb 25, 2021

Summary

Converts unitless number values to strings with units. Allows for use in string- and object-based Emotion styling.

Thinking that we can use the calc approach for now and determine if we want to move to a custom math function as we build more of the theme.

### Checklist

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Feb 25, 2021

This looks good to me in the theme, but when I look out the output, this nesty calcs is a little cringy for me.

"euiScrollBarCorner": "calc(calc(16px * 0.5) * 0.75)"

@thompsongl
Copy link
Contributor Author

this nesty calcs is a little cringy for me

We could roll our own calculation function to use instead. That way the output is always a px string.

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

thompsongl commented Mar 1, 2021

Exploring options for a custom math function and found https://polished.js.org/docs/#math
Even though polished might not be right for us, I think the following is relevant for any math function we would write or use:

In cases where you need to do calculations with mixed units where one unit is a relative length unit, you will want to use CSS Calc.

warning While we've done everything possible to ensure math safely evalutes formulas expressed as strings, you should always use extreme caution when passing math user provided values.

Math on mixed units and user provided values is either risky or not possible. CSS calc is likely the safest choice of, but it might be worth building out more of the theme to see if any mixed/complex/risky cases arise before making a choice.

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

Latest commit results in output like:

  "base": 16,
  "sizes": {
    "euiSize": "16px",
    "euiSizeXS": "4px",
    "euiSizeS": "8px",
    "euiSizeM": "12px",
    "euiSizeL": "24px",
    "euiSizeXL": "32px",
    "euiSizeXXL": "40px",
    "euiButtonMinWidth": "112px",
    "euiScrollBar": "16px",
    "euiScrollBarCorner": "calc(8px * 0.75)"
  },

Note that euiScrollBarCorner still uses calc(), but the alternative would be to write a util to do internal px math. As is, this eliminates nested calc()s and still allows for calculations on calculated values
(euiSize -> euiSizeS -> euiScrollBarCorner).

@kibanamachine
Copy link

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

euiSizeXXL: computed(['sizes.euiSize'], ([euiSize]) => euiSize * 2.5),
euiSize: computed(['base'], ([base]) => `${base}px`),
euiSizeXS: computed(['base'], ([base]) => `${base * 0.25}px`),
euiSizeS: computed(['base'], ([base]) => `${base * 0.5}px`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Has there been more thought on how we might be able to simplify this without needing to write base 3 times?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't put any thought to it until just now. I think at best you're looking at writing base twice, and I'm not sure it scales well beyond top-level keys like base. I can think about it a bit.

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

🎉 This TS based error it awesome!
Screen Shot 2021-03-08 at 16 39 59 PM

I also pushed up a change to the first example to showcase the calc().

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

Will merge this soon so that work based on this branch can move forward

@thompsongl thompsongl merged commit 64cfcd3 into elastic:feature/css-in-js Mar 15, 2021
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.

3 participants