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

Jump to bottom

[CSS-in-JS] Convert EuiMark #4575

Merged
merged 16 commits into from
Mar 15, 2022
Merged

[CSS-in-JS] Convert EuiMark #4575

merged 16 commits into from
Mar 15, 2022

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Feb 24, 2021 •
edited
Loading

Summary

  • Converts EuiMark styles from Sass to Emotion.
  • Establishes a pattern for separating styles by moving to a separate file. (for discussion)
  • Reveals Jest snapshot output as currently configured. (for discussion)

## TODO

- [ ] How to handle test cases with where css is passed to different elements
^ To be done with a component that has multiple styled elements (e.g., EuiAvatar)

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in Chrome, Safari, Edge, and Firefox
  • Added documentation
  • Checked Code Sandbox works for the any docs examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • A changelog entry exists and is marked appropriately

snide
snide reviewed Feb 24, 2021
View reviewed changes
src/components/mark/mark.tsx Outdated Show resolved Hide resolved
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

1 similar comment
@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

jenkins test this

flaky EuiToolTip

@kibanamachine
Copy link

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

@thompsongl thompsongl mentioned this pull request Mar 4, 2021
Closed
10 tasks
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

6 similar comments
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@thompsongl thompsongl mentioned this pull request Jul 30, 2021
Open
24 tasks
@thompsongl thompsongl changed the title [CSS-in-JS] Convert EuiMark [CSS-in-JS] Convert EuiMark Jul 30, 2021
@kibanamachine
Copy link

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

cchaos
cchaos reviewed Aug 15, 2021
View reviewed changes
src/components/mark/__snapshots__/mark.test.tsx.snap Outdated
Comment on lines 4 to 9
.emotion-0 {
background-color: transparent;
font-weight: 700;
color: #343741;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This part is actually quite cool. I was thinking the other day about how I wish we could easily see the differences in the rendered CSS during PR reviews to tell if it's still working as before. The fact that they get added to the snapshots is fantastic for this 🥇

Caveat: Will this happen to every snapshot in consuming apps too? 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

Depends on how they have their snapshot tests setup, but yes. It's possible for consuming apps to use a different serializer that would not add the styles.

Can this be added at a top level for apps? I'd hate to have to add something to every *.test file.

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, it's part of the global jest config

@thompsongl
Copy link
Contributor Author

Will this happen to every snapshot in consuming apps too?

Depends on how they have their snapshot tests setup, but yes. It's possible for consuming apps to use a different serializer that would not add the styles.

@kibanamachine
Copy link

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

@thompsongl thompsongl changed the base branch from feature/css-in-js to master August 26, 2021 20:31
@thompsongl thompsongl changed the base branch from master to css-in-js/breaking September 1, 2021 15:29
@kibanamachine
Copy link

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

@thompsongl
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link

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

cee-chen
cee-chen reviewed Mar 14, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
cee-chen
cee-chen reviewed Mar 14, 2022
View reviewed changes
src/components/mark/mark.tsx
Comment on lines +27 to +28
const useTheme = useEuiTheme();
const styles = euiMarkStyles(useTheme);
Copy link
Member

Choose a reason for hiding this comment

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

DevEx question: why pass a hook into a separate file? Why not just make euiMarkStyles useEuiMarkStyles and call useEuiTheme directly in that file?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, to be clear, I'm proposing this instead:

// mark.tsx
const styles = useEuiMarkStyles();

// mark.styles.ts
import { css } from '@emotion/react';
import { useEuiTheme, transparentize } from '../../services';

export const useEuiMarkStyles = () => {
  const { euiTheme, colorMode } = useEuiTheme();

  return css `// ... etc`;
};

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry, I see you addressed this in https://github.com/elastic/eui/pull/4575/files#r808436110 - I'm still not totally clear on the rationale behind that comment though. Is it a bad thing for a style file to be dependent on React context? TBH, keeping our usage of EUI theming to the style-focused file makes more sense to me from a developer POV.

I'm also very curious what usage would look like with a traditional class component (i.e.: can't use hooks) vs. a functional component. Do we have an example of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did this so that it's possible to just pass in a theme shaped object and get styles, without needing the full React context. No reason we can't do a two-step thing for internal use. That is, keep euiMarkStyles context-agnostic, but add useEuiMarkStyles that is a convenience useEuiTheme() + euiMarkStyles(euiTheme) wrapper.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In more complex cases, we'll probably need to have useEuiTheme in the component file to do dynamic styles with the style prop. In that case this reduces the number of context calls. (More coming on the dynamic styles bit, likely in the EuiAvatar PR).

Copy link
Member

@cee-chen cee-chen Mar 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Gotcha! I think the benefits will be clearer with a dynamic styles example + a class component example (I assume we will need to use a HOC for class components). If we could add those examples to the TBD wiki section along with a basic example that would be amazing!

@kibanamachine
Copy link

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

@thompsongl
Update wiki/creating-components-manually.md
833cd79 
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
@kibanamachine
Copy link

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

cee-chen
cee-chen approved these changes Mar 14, 2022
View reviewed changes
Copy link
Member

@cee-chen cee-chen left a comment

Choose a reason for hiding this comment

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

Documentation looks great! Excited to see more examples get added. Thanks so much for answering my many questions!

@thompsongl thompsongl merged commit e7b3917 into elastic:main Mar 15, 2022
@breehall breehall mentioned this pull request Mar 22, 2022
Merged
@cee-chen cee-chen mentioned this pull request Mar 29, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snide snide snide left review comments

@cchaos cchaos cchaos approved these changes

@cee-chen cee-chen cee-chen approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thompsongl @kibanamachine @cchaos @snide @cee-chen