Skip to content

[CSS-in-JS] Convert mixins#5754

Merged
thompsongl merged 33 commits intoelastic:mainfrom
thompsongl:css-in-js/mixins
Apr 18, 2022
Merged

[CSS-in-JS] Convert mixins#5754
thompsongl merged 33 commits intoelastic:mainfrom
thompsongl:css-in-js/mixins

Conversation

@thompsongl
Copy link
Copy Markdown
Contributor

@thompsongl thompsongl commented Mar 31, 2022 •
edited
Loading

Summary

Convert Sass mixins to JS factories and React hooks

  • Create helper and shadow mixins and hooks
    • Deleted legacy theme shadow mixins from global_styling
    • Amsterdam theme mixins live in its theme directory
  • Create createStyleHookFromMixin
    • Takes a mixin, returns a React hook with theme applied
    • Attempts to handle optional colorMode gracefully

Discuss:

  • Naming patterns
    • euiThemeThing
    • useEuiThemeThing
  • Comparing output to Sass
    • Would some kind of diff with compiled CSS be helpful?

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Mar 31, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 5, 2022

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

@thompsongl thompsongl changed the title [WIP][CSS-in-JS] Convert mixins [CSS-in-JS] Convert mixins Apr 6, 2022
@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented Apr 6, 2022

Opening for review. Will likely write some tests if we feel good about the direction. Not sure if/how we want to document the functions.

@thompsongl thompsongl marked this pull request as ready for review April 6, 2022 20:32
@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Apr 6, 2022

Not sure if/how we want to document the functions.

Can you attempt to update the shadows section of https://elastic.github.io/eui/#/theming/more-tokens#shadow? You should be able to replicate what exists for Sass.

@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented Apr 6, 2022

Can you attempt to update the shadows section

Ah nice; I didn't remember that was there

@thompsongl
Copy link
Copy Markdown
Contributor Author

thompsongl commented Apr 7, 2022

Added some shadow docs!

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 7, 2022

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

cchaos added 12 commits April 8, 2022 10:32
Moved Amsterdam shadow Sass to global_styling
a7d3d78
Clean up old unused Sass parameters
5486afa
Changed names mixin -> eui and remove Styles
c90a57b 
- Added one `useEuiShadow` that accepts t-shirt sizes
- Added `SHADOW_SIZE` and `_EuiShadowSizes` global types of t-shirt sizes
- Simplified docs section to loop through t-shirt sizes
- Consolidated `shadowOpacity` into `getShadowColor`
More docs and missed rename
f89ced8
More docs fixes
10de00c
Moved canAnimate functions to global animations file and added docs
5c11e4f
Changed mixinScreenReaderOnlyStyles to be the `euiScreenReaderOnlyS…
36a0cbb 
…tyles` component styles and added docs
Consolidated overflow scrolling utilites to one new page
8791260
Removed Sass mixin innerBorder and moved to specific use-case of `e…
bb56bf0 
…uiColorPaletteInnerBorder`
More organization
d467cbd
More organization
8f9d417
More docs and added class utility .eui-scrollBar
2a1716d
@cchaos cchaos mentioned this pull request Apr 11, 2022
Merged
11 tasks
@cchaos
Copy link
Copy Markdown
Contributor

cchaos commented Apr 11, 2022

PR4U: thompsongl#16

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 12, 2022

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

@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 12, 2022

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

chandlerprall
chandlerprall approved these changes Apr 18, 2022
View reviewed changes
Copy link
Copy Markdown
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.

I really like how the methods break down, and I think it covers a lot of precedent-setting that we can follow moving forward. Had a couple thoughts on the docs side of things, but the EUI code LGTM.

src/global_styling/variables/_animations.ts
* @param content
* @returns string
*/
export const euiCanAnimate = (content: string) => `
Copy link
Copy Markdown
Contributor

@chandlerprall chandlerprall Apr 18, 2022

Choose a reason for hiding this comment

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

Have you explored Emotion's InterpolationPrimitive type and if it, a subset, or this string is the right choice here and in other utility methods?

export type InterpolationPrimitive =
  | null
  | undefined
  | boolean
  | number
  | string
  | ComponentSelector
  | Keyframes
  | SerializedStyles
  | CSSObject

Copy link
Copy Markdown
Contributor Author

@thompsongl thompsongl Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I hadn't seen this but it might make sense in some cases. We'd need add @emotion/serialize as a dependency to use it, so I'm ok going with the more restrictive string here for now.

@thompsongl thompsongl enabled auto-merge (squash) April 18, 2022 20:43
@kibanamachine
Copy link
Copy Markdown

kibanamachine commented Apr 18, 2022

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

@thompsongl thompsongl merged commit cffdce0 into elastic:main Apr 18, 2022
@cchaos cchaos mentioned this pull request Apr 19, 2022
Closed
@thompsongl thompsongl mentioned this pull request Apr 19, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@chandlerprall chandlerprall chandlerprall approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@thompsongl @kibanamachine @cchaos @chandlerprall