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

[EuiDataGrid] Add lineHeight configuration to rowHeightsOptions #5221

Merged
merged 8 commits into from
Sep 30, 2021

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Sep 28, 2021

Summary

Original concept in #5140 and heavy lifting done by @chandlerprall, I'm just carrying this over the finish line in terms of opening up a PR.

Documentation example with lineHeight: '2em':

Example with lineHeight: '1' (unitless):

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile

  • Checked in Chrome, Safari, Edge, and Firefox

- [ ] Props have proper autodocs and playground toggles

- [ ] Checked Code Sandbox works for any docs examples
- [ ] Added or updated jest tests (Skipped because I've yet to write Jest tests for row_height_utils)
- [ ] Checked for breaking changes and labeled appropriately
- [ ] Checked for accessibility including keyboard-only and screenreader modes

  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

chandlerprall and others added 4 commits September 28, 2021 14:14
- Add new example that uses same dataset as other 2 on page, but makes it text only (lineHeight doesn't work as expected with nested <EuiText>

- Update existing documentation, make 'fixed' example more of a demo of rowHeights overrides
- Fix awkward grammar which was tweaked in another spot in the docs

- fix defaultHeight snippet example that was missing a closing brace (change it to static #, since I added a lineCount example already earlier in the page)

- fix redundant =
@cee-chen cee-chen marked this pull request as ready for review September 28, 2021 21:15
@cee-chen
Copy link
Member Author

FYI @ryankeairns and @andreadelrio, thanks for your patience! This should hopefully be available next release

@cee-chen
Copy link
Member Author

jenkins test this

@kibanamachine
Copy link

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

@miukimiu miukimiu self-requested a review September 29, 2021 16:03
@cee-chen
Copy link
Member Author

jenkins test this

@kibanamachine
Copy link

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

Copy link
Contributor

@miukimiu miukimiu left a comment

Choose a reason for hiding this comment

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

Thanks, @constancecchen! LGTM! 🎉

Tested in Chrome, Safari, Edge, and Firefox. Also did some tests locally.

Just added small suggestions to the docs.

I also noticed that on the following page the rowHeightsOptions prop is missing in both snippet and general props: https://eui.elastic.co/pr_5221/#/tabular-content/data-grid#snippet-with-every-feature-in-use.

@kibanamachine
Copy link

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

@cee-chen
Copy link
Member Author

55a1069 should address

I also noticed that on the following page the rowHeightsOptions prop is missing in both snippet and general props: https://eui.elastic.co/pr_5221/#/tabular-content/data-grid#snippet-with-every-feature-in-use.

@cee-chen cee-chen enabled auto-merge (squash) September 30, 2021 16:26
@kibanamachine
Copy link

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

@cee-chen cee-chen merged commit 9a5b78d into elastic:master Sep 30, 2021
@cee-chen cee-chen deleted the datagrid-lineheight branch September 30, 2021 17:02
@ryankeairns
Copy link
Contributor

Thanks @constancecchen !

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.

5 participants