Skip to content

[EuiDataGrid] Fix large density not increasing font size on Amsterdam theme#5320

Merged
cee-chen merged 6 commits intoelastic:masterfrom
cee-chen:datagrid-density-large
Oct 27, 2021
Merged

[EuiDataGrid] Fix large density not increasing font size on Amsterdam theme#5320
cee-chen merged 6 commits intoelastic:masterfrom
cee-chen:datagrid-density-large

Conversation

@cee-chen
Copy link
Copy Markdown
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
Copy Markdown

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

@cee-chen
Copy link
Copy Markdown
Contributor Author

jenkins test this

@kibanamachine
Copy link
Copy Markdown

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

@kibanamachine
Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown

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

@kibanamachine
Copy link
Copy Markdown

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

Copy link
Copy Markdown
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
Copy Markdown
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