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

[EuiTableRow] Convert to Emotion #7628

Merged
merged 9 commits into from
Mar 28, 2024
Merged

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 27, 2024

Summary

NOTE: This is going into the EuiTable Emotion conversion/cleanup feature branch.

This doesn't convert all table row styles to Emotion, but it converts (what I consider) to be the majority of row-related CSS (enough to get a mostly functional Storybook up). There are still some styles that I consider belong to cells, rather than rows, which I will target in the next cell-focused PR.

As mentioned in #7625, staging itself will still look broken until all CSS is converted over (at which point I'll do another prod vs staging QA pass and also hopefully have more Storybooks written by then for easier QA).

⚠️ Removals/changes that will affect consumers (checked for zero usages in Kibana):

  • Removed Sass @euiTableActionsBackgroundMobile mixin
  • Removed Sass $euiTableActionsBorderColor variable (+ updated mobile usages to use the theme-wide border color token, matching desktop border colors)
  • Removed Sass $euiTableHoverColor variable
  • Removed Sass $euiTableSelectedColor variable
  • Removed Sass $euiTableHoverSelectedColor variable
  • Removed Sass $euiTableHoverClickableColor variable
  • Removed Sass $euiTableFocusClickableColor variable

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes (Note that Storybook on dark mode will look semi broken due to cell styles not yet being converted to Emotion)
    • Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA - N/A
  • Code quality checklist
    • Added or updated a component Story
      - [ ] Added or updated jest and cypress tests
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A - mobile tables aren't a thing in Figma

- just setting up the responsive context and conditional logic, no styles yet
- make color variables a utility fn to help reduce cruft/setup before the styles obj (there will be more coming)
+ remove now-unnecessary `:hover` override, euiPanel mixin

+ remove unnecessary extra `euiBottomShadowSmall` on expanded rows, that already gets inherited by `.euiTableRow`

- TODO: `cellContentPadding` might need to be DRYed out with `euiTable`, but let's hold off on that for now
- note that we've put `position: relative` on all mobile rows as a default since so many states use it - so no need to apply it to only specific kinds of rows
- ⚠️ Opinionatedly remove `$euiTableActionsBorderColor` in favor of just using the border color - I don't see a point at all in making this distinction

- prefer a pseudo element over a linear gradient for rendering the 'border'

- remove mixins and variables
- note: the `_` prefix before the animation util fn prevents it from getting a label in Emotion's className logic
+ add one mobile fix and one temporary !important workaround found from said QA
@cee-chen cee-chen changed the title [EuiTableRow] Convert majority of styles to Emotion [EuiTableRow] Convert to Emotion Mar 27, 2024
- table cells will use these shortly, so we might as well make it now
@cee-chen cee-chen marked this pull request as ready for review March 27, 2024 23:47
@cee-chen cee-chen requested a review from a team as a code owner March 27, 2024 23:47
@kibanamachine
Copy link

Preview staging links for this PR:

@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

I tried as best as possible to follow along what's happening for tables, but I might still miss some general understanding 😅 But overall the changes look good to me, I left a couple smaller notes.

src/components/table/table_row.styles.ts Show resolved Hide resolved
src/components/table/table_row.styles.ts Outdated Show resolved Hide resolved
src/components/table/table_row.stories.tsx Show resolved Hide resolved
src/components/table/table_row.stories.tsx Show resolved Hide resolved
src/components/table/table.styles.ts Show resolved Hide resolved
src/components/table/table_row.stories.tsx Show resolved Hide resolved
Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

🚢 🐈‍⬛ Nice work on converting the code but also making it cleaner more readable ✨

@cee-chen
Copy link
Contributor Author

Thanks as always for the awesome and thorough PR reviews Lene! 💖

@cee-chen cee-chen merged commit af5698c into elastic:tableflip Mar 28, 2024
6 checks passed
@cee-chen cee-chen deleted the emotion/table-4 branch March 28, 2024 17:27
cee-chen added a commit that referenced this pull request Mar 28, 2024
cee-chen added a commit that referenced this pull request Apr 1, 2024
cee-chen added a commit that referenced this pull request Apr 5, 2024
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