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.

Sign up for GitHub

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] Simplify shadow parameters #5892

Merged
merged 6 commits into from
May 16, 2022
Merged

[CSS-in-JS] Simplify shadow parameters #5892

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aad3eb0
Simplify shadow parameters
May 10, 2022
db807fa
cl
May 10, 2022
3e8d27c
Update naming and simplify some destructuring
May 11, 2022
fb3cd34
One more
May 11, 2022
d0c3cfe
Apply suggestions from code review
cchaos May 11, 2022
49938a9
Merge branch 'elastic:main' into css-in-js/simplify-shadow-mixins
cchaos May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion src-docs/src/views/theme/other/_shadow_js.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
import { getPropsFromComponent } from '../../../services/props/get_props';
import { getDescription } from '../../../services/props/get_description';

// TODO: Update imports
import {
useEuiShadow,
useEuiShadowFlat,
Expand Down
66 changes: 35 additions & 31 deletions src/components/bottom_bar/bottom_bar.styles.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,34 +23,38 @@ const euiBottomBarAppear = keyframes`
}
`;

export const euiBottomBarStyles = ({ euiTheme, colorMode }: UseEuiTheme) => ({
// Base
// Text color needs to be reapplied to properly scope the forced `colorMode`
euiBottomBar: css`
${euiShadowFlat(euiTheme, undefined, colorMode)};
background: ${shade(euiTheme.colors.lightestShade, 0.5)};
color: ${euiTheme.colors.text};
${euiCanAnimate} {
animation: ${euiBottomBarAppear} ${euiTheme.animation.slow}
${euiTheme.animation.resistance};
}
`,
static: css``,
fixed: css`
z-index: ${Number(euiTheme.levels.header) - 2};
`,
sticky: css`
z-index: ${Number(euiTheme.levels.header) - 2};
`,
// Padding
s: css`
padding: ${euiTheme.size.s};
`,
m: css`
padding: ${euiTheme.size.base};
`,
l: css`
padding: ${euiTheme.size.l};
`,
none: '',
});
export const euiBottomBarStyles = (_euiTheme: UseEuiTheme) => {
const { euiTheme } = _euiTheme;
Copy link
Contributor

@cee-chen cee-chen May 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This distinction feels somewhat confusing to me and I wish we were a bit more consistent on whether we pass the entire returned context or just euiTheme between various helpers (e.g., the typography fns all pass euiTheme.euiTheme and not the parent context, but this is also because basically every single .styles.ts file deconstructs { euiTheme } automatically in its params).

I also wish we had a clearer naming convention for the context vs the actual theme data so that we don't end up essentially deconstructing _euiTheme.euiTheme 🙈 Maybe themeContext.euiTheme instead?...

@thompsongl, any thoughts here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed with all those points. I wonder if it's just easiest to say, any mixin/function should accept the entirety of UseEuiTheme whether they only need the .euiTheme portion or not? It keeps them scalable in case of future needs to grab the colorMode.

As for naming we could just change to also ensure we're naming it as lowercase useEuiTheme so that it is clear.

Suggested change
export const euiBottomBarStyles = (_euiTheme: UseEuiTheme) => {
const { euiTheme } = _euiTheme;
export const euiBottomBarStyles = (useEuiTheme: UseEuiTheme) => {
const { euiTheme } = useEuiTheme;

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I had mixins passing the full result of useEuiTheme originally. The cases where mixins needed colorMode were scarce so I went the other way. But I'd rather see consistency like you've suggested if working with the current mixin shape is difficult.

We should update all mixins and the createStyleHookFromMixin util with the new pattern.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++

Also I just realized we could simplify the code portion a bit too by deconstructing in the parameters.

Suggested change
export const euiBottomBarStyles = (_euiTheme: UseEuiTheme) => {
const { euiTheme } = _euiTheme;
export const euiBottomBarStyles = ({ euiTheme }: UseEuiTheme) => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I pushed a commit based on those latest comments. But I didn't touch the util (that one still confuses me a bit)

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not 😸

😄 But are you ok if I do it and add a bunch to this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or if you want to merge this I can do a follow up. Up to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, go for it. I'm out for the rest of the week.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm not sure I can make a generic function that works with mixins that do have required args and mixins that do not have required args.

const euiMixin(
  euiTheme: UseEuiTheme;
  required: PropertyThatIsRequired;
  optional?: {
    optionalKey1?: something;
    optionalKey2?: something;
  }
) => {}

vs

const euiMixin(
  euiTheme: UseEuiTheme;
  optional?: {
    optionalKey1?: something;
    optionalKey2?: something;
  }
) => {}

So a couple options:

  • Only have the former and you're required to pass an empty arg when no properties are required: euiScrollBarStyles(euiThemeContext, {}, {size: 20});
  • Move the required args and optional args into a single object:
const euiMixin(
  euiTheme: UseEuiTheme;
  options: {
    requiredKey1: something;
    optionalKey1?: something;
    optionalKey2?: something;
  }
) => {}
  • Create a second version of createStyleHookFromMixin so we have one with required args and one without
  • Scrap createStyleHookFromMixin entirely and manually create hooks. (We lose the type safety/consistency that it adds to mixins; we have to just be consistent when creating new mixins).

Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I think my (opinionated) vote goes to

Scrap createStyleHookFromMixin entirely and manually create hooks. (We lose the type safety/consistency that it adds to mixins; we have to just be consistent when creating new mixins).

I haven't used this helper personally despite having created a few mixins and I don't super see the need for it. 🤔 It's not really that tedious to manually write a 5-line hook next to a mixin.

If we do strongly want the helper still, then my next vote goes to Move the required args and optional args into a single object

cchaos marked this conversation as resolved.
Show resolved Hide resolved

return {
// Base
// Text color needs to be reapplied to properly scope the forced `colorMode`
euiBottomBar: css`
${euiShadowFlat(_euiTheme)};
cchaos marked this conversation as resolved.
Show resolved Hide resolved
background: ${shade(euiTheme.colors.lightestShade, 0.5)};
color: ${euiTheme.colors.text};
${euiCanAnimate} {
animation: ${euiBottomBarAppear} ${euiTheme.animation.slow}
${euiTheme.animation.resistance};
}
`,
static: css``,
fixed: css`
z-index: ${Number(euiTheme.levels.header) - 2};
`,
sticky: css`
z-index: ${Number(euiTheme.levels.header) - 2};
`,
// Padding
s: css`
padding: ${euiTheme.size.s};
`,
m: css`
padding: ${euiTheme.size.base};
`,
l: css`
padding: ${euiTheme.size.l};
`,
none: '',
};
};
106 changes: 50 additions & 56 deletions src/themes/amsterdam/global_styling/mixins/shadow.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@

import { useEuiTheme, UseEuiTheme } from '../../../../services/theme';
import { getShadowColor } from '../functions';
import { createStyleHookFromMixin } from '../../../../global_styling/utils';
import {
_EuiThemeShadowSize,
_EuiThemeShadowCustomColor,
Expand All @@ -22,11 +21,10 @@ export interface EuiShadowCustomColor {
* euiSlightShadow
*/
export const euiShadowXSmall = (
{ colors }: UseEuiTheme['euiTheme'],
{ color: _color }: _EuiThemeShadowCustomColor = {},
colorMode: UseEuiTheme['colorMode']
{ euiTheme, colorMode }: UseEuiTheme,
{ color: _color }: _EuiThemeShadowCustomColor = {}
) => {
const color = _color || colors.shadow;
const color = _color || euiTheme.colors.shadow;
return `
box-shadow:
0 .8px .8px ${getShadowColor(color, 0.04, colorMode)},
Expand All @@ -38,11 +36,10 @@ box-shadow:
* bottomShadowSmall
*/
export const euiShadowSmall = (
{ colors }: UseEuiTheme['euiTheme'],
{ color: _color }: _EuiThemeShadowCustomColor = {},
colorMode: UseEuiTheme['colorMode']
{ euiTheme, colorMode }: UseEuiTheme,
{ color: _color }: _EuiThemeShadowCustomColor = {}
) => {
const color = _color || colors.shadow;
const color = _color || euiTheme.colors.shadow;
return `
box-shadow:
0 .7px 1.4px ${getShadowColor(color, 0.07, colorMode)},
Expand All @@ -55,11 +52,10 @@ box-shadow:
* bottomShadowMedium
*/
export const euiShadowMedium = (
{ colors }: UseEuiTheme['euiTheme'],
{ color: _color }: _EuiThemeShadowCustomColor = {},
colorMode: UseEuiTheme['colorMode']
{ euiTheme, colorMode }: UseEuiTheme,
{ color: _color }: _EuiThemeShadowCustomColor = {}
) => {
const color = _color || colors.shadow;
const color = _color || euiTheme.colors.shadow;
return `
box-shadow:
0 .9px 4px -1px ${getShadowColor(color, 0.08, colorMode)},
Expand All @@ -73,12 +69,10 @@ box-shadow:
* bottomShadow
*/
export const euiShadowLarge = (
{ colors }: UseEuiTheme['euiTheme'],
{ color: _color }: _EuiThemeShadowCustomColor = {},
colorMode: UseEuiTheme['colorMode']
{ euiTheme, colorMode }: UseEuiTheme,
{ color: _color }: _EuiThemeShadowCustomColor = {}
) => {
const color = _color || colors.shadow;

const color = _color || euiTheme.colors.shadow;
return `
box-shadow:
0 1px 5px ${getShadowColor(color, 0.1, colorMode)},
Expand All @@ -95,12 +89,10 @@ export interface EuiShadowXLarge extends _EuiThemeShadowCustomColor {
reverse?: boolean;
}
export const euiShadowXLarge = (
{ colors }: UseEuiTheme['euiTheme'],
{ color: _color, reverse }: EuiShadowXLarge = {},
colorMode: UseEuiTheme['colorMode']
{ euiTheme, colorMode }: UseEuiTheme,
{ color: _color, reverse }: EuiShadowXLarge = {}
) => {
const color = _color || colors.shadow;

const color = _color || euiTheme.colors.shadow;
return `
box-shadow:
0 ${reverse ? '-' : ''}2.7px 9px ${getShadowColor(color, 0.13, colorMode)},
Expand All @@ -114,11 +106,10 @@ box-shadow:
* TODO: I think this is only used by panels/cards in the Amsterdam theme, move there
*/
export const euiSlightShadowHover = (
{ colors }: UseEuiTheme['euiTheme'],
{ color: _color }: _EuiThemeShadowCustomColor = {},
colorMode: UseEuiTheme['colorMode']
{ euiTheme, colorMode }: UseEuiTheme,
{ color: _color }: _EuiThemeShadowCustomColor = {}
) => {
const color = _color || colors.shadow;
const color = _color || euiTheme.colors.shadow;
return `
box-shadow:
0 1px 5px ${getShadowColor(color, 0.1, colorMode)},
Expand All @@ -127,9 +118,12 @@ box-shadow:
0 23px 35px ${getShadowColor(color, 0.05, colorMode)};
`;
};
export const useEuiSlightShadowHover = createStyleHookFromMixin(
euiSlightShadowHover
);
export const useEuiSlightShadowHover = (
color?: _EuiThemeShadowCustomColor['color']
) => {
const euiThemeContext = useEuiTheme();
return euiSlightShadowHover(euiThemeContext, { color });
};

/**
* bottomShadowFlat
Expand All @@ -138,52 +132,52 @@ export const useEuiSlightShadowHover = createStyleHookFromMixin(
* Useful for popovers that drop UP rather than DOWN.
*/
export const euiShadowFlat = (
{ colors }: UseEuiTheme['euiTheme'],
color: _EuiThemeShadowCustomColor['color'] = undefined,
colorMode: UseEuiTheme['colorMode']
{ euiTheme, colorMode }: UseEuiTheme,
{ color: _color }: _EuiThemeShadowCustomColor = {}
) => {
const _color = color || colors.shadow;
const color = _color || euiTheme.colors.shadow;
return `
box-shadow:
0 0 .8px ${getShadowColor(_color, 0.06, colorMode)},
0 0 2px ${getShadowColor(_color, 0.04, colorMode)},
0 0 5px ${getShadowColor(_color, 0.04, colorMode)},
0 0 17px ${getShadowColor(_color, 0.03, colorMode)};
0 0 .8px ${getShadowColor(color, 0.06, colorMode)},
0 0 2px ${getShadowColor(color, 0.04, colorMode)},
0 0 5px ${getShadowColor(color, 0.04, colorMode)},
0 0 17px ${getShadowColor(color, 0.03, colorMode)};
`;
};
export const useEuiShadowFlat = createStyleHookFromMixin(euiShadowFlat);
export const useEuiShadowFlat = (
color?: _EuiThemeShadowCustomColor['color']
) => {
const euiThemeContext = useEuiTheme();
return euiShadowFlat(euiThemeContext, { color });
};

// One hook to rule them all
interface EuiShadowStyles {
size?: _EuiThemeShadowSize;
color?: _EuiThemeShadowCustomColor['color'];
}
export const euiShadow = (
euiTheme: UseEuiTheme['euiTheme'],
{ size = 'l', color = undefined }: EuiShadowStyles = {},
colorMode: UseEuiTheme['colorMode']
euiThemeContext: UseEuiTheme,
size: _EuiThemeShadowSize = 'l',
{ color }: _EuiThemeShadowCustomColor = {}
) => {
switch (size) {
case 'xs':
return euiShadowXSmall(euiTheme, { color }, colorMode);
return euiShadowXSmall(euiThemeContext, { color });
case 's':
return euiShadowSmall(euiTheme, { color }, colorMode);
return euiShadowSmall(euiThemeContext, { color });
case 'm':
return euiShadowMedium(euiTheme, { color }, colorMode);
return euiShadowMedium(euiThemeContext, { color });
case 'l':
return euiShadowLarge(euiTheme, { color }, colorMode);
return euiShadowLarge(euiThemeContext, { color });
case 'xl':
return euiShadowXLarge(euiTheme, { color }, colorMode);
return euiShadowXLarge(euiThemeContext, { color });

default:
console.warn('Please provide a valid size option to useEuiShadow');
return '';
}
};

export const useEuiShadow = (
size: EuiShadowStyles['size'] = 'l',
color?: EuiShadowStyles['color']
size: _EuiThemeShadowSize = 'l',
color?: _EuiThemeShadowCustomColor['color']
) => {
const { euiTheme, colorMode } = useEuiTheme();
return euiShadow(euiTheme, { size, color }, colorMode);
const euiThemeContext = useEuiTheme();
return euiShadow(euiThemeContext, size, { color });
};
2 changes: 2 additions & 0 deletions upcoming_changelogs/5892.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Updated all CSS-in-JS shadow functions parameters to match a `(euiTheme, { color? })` order
- Updated `euiShadow()` parameters to `(euiTheme, size, { color? })`