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

[Emotion] Convert EuiBottomBar to Emotion #5823

Merged
merged 19 commits into from
Apr 29, 2022

Conversation

breehall
Copy link
Contributor

@breehall breehall commented Apr 20, 2022

Summary

Converts EuiBottomBar to Emotion
Additional changes include converting the original EuiBottomBar function into an internal utility component that is nested inside of a new component that wraps the the bottom bar in an EuiThemeProvider. This was done to lay the ground work for future enhancements.
Future enhancements include resolving #4158 by using colorMode: inverse on the bottom bar. This will ensure that the bottom bar always stands out in light mode and in dark mode because the background color of the bar will always be in contrast to the color mode.
This will also include adding a prop to EuiBottomBar that will consumers to choose light or dark as the colorMode for the bottom bar. If this prop is in use, the colorMode: inverse enhancement (mentioned above) will not take affect.
These enhancements will come at a later time to reduce the amount of disruption for consumers. The colorMode changes will affect the background color of the bottom bar, but until other components have been converted to Emotion, these changes will be out of sync.

Testing Note
With the changes made, the background color of EuiBottomBar should be a dark gray (euiTheme.colors.lightShade) in both light and dark mode. Previous, the background color was white.

Things to look out for when moving styles

  • Anything in the backlog that could be a quick fix for that component?
  • Convert global Sass vars to JS version like sizing
  • Convert component-specific Sass vars to exported JS versions
  • Convert side specific padding, margin, and position to -inline and -block (Logical properties)
  • Reduce specificity where possible (usually by reducing class name chaining)
  • Use gap property to add margin between items if using flex
  • Can any still existing .js files be converted to TS?

QA

  • Do className, css, and style props work properly when passed by the consumer
  • Does the rendered class read as expected

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
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@breehall breehall changed the title Emotion bottom bar [Emotion] Convert EuiBottomBar to Emotion Apr 20, 2022
… that wraps the original inside of EuiThemeProvider to pass the dark theme to the entire component.

Removed the shouldRenderCustomStyles test utility
@breehall breehall marked this pull request as ready for review April 26, 2022 20:50
// Base
euiBottomBar: css`
${euiShadowFlat(euiTheme, undefined, colorMode)};
background: ${euiTheme.colors.lightShade};
Copy link
Contributor

Choose a reason for hiding this comment

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

We still need this background color to render as it did previously. The sass variable is:

$euiHeaderDarkBackgroundColor: lightOrDarkTheme(shade($euiColorDarkestShade, 28%), shade($euiColorLightestShade, 50%)) !default;

But since we're only ever in dark theme, we just need to apply the second parameter as the color:

Suggested change
background: ${euiTheme.colors.lightShade};
background: ${shade(euiTheme.colors.lightestShade, .5)};

color: ${euiTheme.colors.text};
animation: ${euiBottomBarAppear} ${euiTheme.animation.slow}
${euiTheme.animation.resistance};
z-index: ${_euiBottomBarZHeader - 2};
Copy link
Contributor

Choose a reason for hiding this comment

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

I just pushed the z-index layering global theme vars to main.

Suggested change
z-index: ${_euiBottomBarZHeader - 2};
z-index: ${euiTheme.levels.header - 2};

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, it looks like this z-index should also only apply if the position is fixed or sticky.

Comment on lines 13 to 14
//const _euiBottomBarColor = '#131317';
const _euiBottomBarZHeader = 1000;
Copy link
Contributor

Choose a reason for hiding this comment

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

So neither of these should be needed anymore.

Suggested change
//const _euiBottomBarColor = '#131317';
const _euiBottomBarZHeader = 1000;

}

100% {
transform: translateY(00%);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
transform: translateY(00%);
transform: translateY(0%);

src/components/bottom_bar/bottom_bar.styles.ts Outdated Show resolved Hide resolved
src/components/bottom_bar/bottom_bar.styles.ts Outdated Show resolved Hide resolved
src/components/index.scss Outdated Show resolved Hide resolved
@@ -156,6 +161,13 @@ export const EuiBottomBar = forwardRef<
className
);

const cssStyles = [
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 added styles[position] back to the cssStyles array because I need to access the position to determine if the z-index should be added to the bottom bar. Since I already need it for that purpose, should we just attach the position styling in those style blocks instead of passing it in directly here?

@breehall breehall requested a review from cchaos April 27, 2022 20:06
@cchaos
Copy link
Contributor

cchaos commented Apr 28, 2022

Jenkins, test this

@kibanamachine
Copy link

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

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.

🙌 Our first dark-theme only component!

Just to compare using already converted components (Avatar, HR, LoadingChart) rendered inside the bottom bar:

Global light mode
Screen Shot 2022-04-28 at 13 49 59 PM

Global dark mode
Screen Shot 2022-04-28 at 13 50 13 PM


LGTM. I tested in the 3 browsers and mobile. Gonna tag @thompsongl in on this one to ensure the wrapping component stuff is all what he expected.

Comment on lines +231 to +240
export const EuiBottomBar = forwardRef<HTMLElement, EuiBottomBarProps>(
(props, ref) => {
const BottomBar = _EuiBottomBar;
return (
<EuiThemeProvider colorMode={'dark'}>
<BottomBar ref={ref} {...props} />
</EuiThemeProvider>
);
}
);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you document this as a pattern for color-mode specific components in the Emotion styling wiki???

@cchaos cchaos requested a review from thompsongl April 28, 2022 17:56
Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Just a couple nits. Nice work!


export const euiBottomBarStyles = ({ euiTheme, colorMode }: UseEuiTheme) => ({
// Base
//Text color needs to be reapplied to properly scope the forced `colorMode`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
//Text color needs to be reapplied to properly scope the forced `colorMode`
// Text color needs to be reapplied to properly scope the forced `colorMode`

Comment on lines 11 to 12
import { UseEuiTheme } from '../../services';
import { shade } from '../../services/color';
Copy link
Contributor

Choose a reason for hiding this comment

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

Can these be combined?

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

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.

4 participants