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] Fix large density not increasing font size on Amsterdam theme #5320

Merged
merged 6 commits into from
Oct 27, 2021

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Oct 25, 2021

Summary

euiFontSize is 14px on Amsterdam but 16px on legacy - we should specifyeuiFontSizeM if we want a 16px font size on expanded density data grids.

Before

before.mp4

After

after.mp4

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
- [ ] 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

  • A changelog entry exists and is marked appropriately

@kibanamachine
Copy link

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

@cee-chen
Copy link
Contributor Author

jenkins test this

@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

@elizabetdev elizabetdev self-requested a review October 26, 2021 10:34
Copy link
Contributor

@elizabetdev elizabetdev left a comment

Choose a reason for hiding this comment

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

Tested in Chrome, Safari, Edge, and Firefox. LGTM! 🎉

@cchaos
Copy link
Contributor

cchaos commented Oct 26, 2021

Oh shoot, @miukimiu's right. Sorry @constancecchen I gave you bad intel. I do think jumping from 16px to 18 is a bit drastic though, so I'd recommend applying the change in teh Amsterdam overrides like she suggested. Sorry ☹️

@cee-chen cee-chen requested a review from elizabetdev October 26, 2021 21:19
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

Copy link
Contributor

@elizabetdev elizabetdev 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! Tested again and LGTM! 🎉

@cee-chen
Copy link
Contributor Author

Wahoo! Thanks y'all for learning me some theming differences!

@cee-chen cee-chen merged commit 3326d61 into elastic:master Oct 27, 2021
@cee-chen cee-chen deleted the datagrid-density-large branch October 27, 2021 16:22
ym pushed a commit to ym/eui that referenced this pull request Oct 29, 2021
… theme (elastic#5320)

* Fix large density not increasing in fontSize on Amsterdam theme

* Add changelog entry

* Switch to Amsterdam override
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.

4 participants