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 EuiDataGrid gridStyles to Emotion (Part 3) #8006

Merged
merged 18 commits into from
Sep 9, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 5, 2024

Summary

Part of #6394

This PR looks intimidating in terms of file diff, but I promise the majority of it is just Storybooks! As such I strongly recommend following along by commit. The TL;DR of changes:

  • Converts styles impacted by gridStyle to Emotion (rows, borders, and cell density)
  • Addresses a long-standing TODO in EuiDataGrid where we were using static numbers to calculate row height to inherit dynamically from our EUI theme tokens (637a2ba)
  • Fixes border style permutations that look broken on prod, mainly around underline/overline header & footers (30c6f9b?w=1)
  • A ton of Storybook cleanup (last 4-5 commits):
    • I wanted a way to get the nested objects in Storybook's controls table, so I created a separate stories.utils.tsx file as a way to generate fake 'components' that would fool Storybook's react docgen, as well as to share a stateful datagrid:
    This lets us easily reuse the same dataset and columns etc across multiple stories and story subdirectories. And IMO, EuiDataGrid is well complex enough to warrant multiple `.stories` files 🫠

QA

General checklist

  • Browser QA
    • Checked in both light and dark modes
    • Checked in Chrome, Safari, Edge, and Firefox
      - [ ] Checked in mobile
      - [ ] Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
    • Updated documentation
    • Props have proper autodocs (using @default if default values are missing)
      - [ ] Checked Code Sandbox works for any docs examples
  • Code quality checklist
  • 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

@cee-chen cee-chen force-pushed the emotion/datagrid-1 branch 2 times, most recently from 5d04264 to f65e427 Compare September 5, 2024 17:57
@cee-chen cee-chen changed the title [Emotion] Convert EuiDataGrid grid styles to Emotion (part 3) [Emotion] Convert EuiDataGrid gridStyles to Emotion (part 3) Sep 5, 2024
@cee-chen cee-chen force-pushed the emotion/datagrid-1 branch 2 times, most recently from 6afaa83 to d7d7e53 Compare September 6, 2024 01:23
@cee-chen cee-chen changed the title [Emotion] Convert EuiDataGrid gridStyles to Emotion (part 3) [Emotion] Convert EuiDataGrid gridStyles to Emotion (Part 3) Sep 6, 2024
- unfortunately can't be as specific here, has to be applied to top level datagrid styles
- handle this at topmost grid level, I personally don't think it makes any sense to split it up by component when they affect each other - more files makes it harder to write/predict

+ clean up several CSS unsets by assuming lack of borders rather than presence of borders
- some fontSize/lineHeight tweaking required to stay the same as prod (due to changes Caroline made to euiFontSize)

+ improve Emotion className labeling
- we can now obtain cell padding sizes from dynamic themes, hooray
- pass `gridStyles` down through the body and to the rows so CSS can be a bit flatter, as those styles only concern those components

+ convert enzyme tests to RTL
+ remove unnecessary show footer toggle - it's the entire point of the demo, why would we disable it?

+ remove flex in favor of EuiSpacer
- grid borders and header/footer underline/overlines were conflicting with one another

- borders on header/footer cells were adding extra height to the cells themselves (unlike regular row cells)
…utils

- VRT stories should use minimal data still for screenshot simplicity

+ add 2 new stories with more customizable controls, which lists the nested props documentation for each nested config
- allows us to expand props documentation & use controls for `rowHeightsOptions`

+ add test for lineHeight
@cee-chen cee-chen marked this pull request as ready for review September 6, 2024 01:49
@cee-chen cee-chen requested a review from a team as a code owner September 6, 2024 01:49
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.

Awesome work on the conversion, you added some nice little code improvements and I didn't see any unexpected regression 🎉
And I really dig the storybook organization update 👏 I left a couple small questions, but nothing major.

+ remove `PRESERVED_FALSE_VALUE_PROPS` logic - we should let the defaultProps logic handle that instead and always show `false` props if they're non-defaults (testable on, e.g. `<EuiExpression uppercase={false} />`)
@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.

🚢 🐈‍⬛ Thanks for the updates, all looking good to me now 👍

@cee-chen cee-chen merged commit e1075f0 into elastic:main Sep 9, 2024
5 checks passed
@cee-chen cee-chen deleted the emotion/datagrid-1 branch September 9, 2024 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants