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.

Sign up for GitHub

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

Jump to bottom

feat(heatmap): reduce font size to fit label within cells #1352

Merged
merged 17 commits into from
Sep 8, 2021
Merged

feat(heatmap): reduce font size to fit label within cells #1352

merged 17 commits into from
Sep 8, 2021

Conversation

markov00
Copy link
Member

@markov00 markov00 commented Sep 2, 2021

Summary

We introduced the ability to scale the font size of numeric values within the heatmap cells if the label doesn't fit within the label using the default font size. The resulting font size is constrained within a user-configured range. If the label doesn't fit using the min font size, the label is not rendered.

BREAKING CHANGE

The config.label.fontSize prop is replaced by config.label.minFontSize and config.label.maxFontSize. You can specify the same value for both properties to have a fixed font size.
The config.label.align and config.label.baseline props are removed from the HeatmapConfig object. These props handle the alignment of the text within the text and they are not currently compatible with the font size scaling feature introduced. Is not very common to change the alignment of text within rectangular boxes where the centered option is the most used one.

Screenshot 2021-09-02 at 11 25 28

Details

The changes are actually suboptimal:

  • from the API point of view we can have a single union type for the fontSize like Pixels | [minFontSize: Pixels, maxFontSize:Pixels] or something along those lines. Is a union type but it should be converted to the extended form after the API layer.
  • The value volatility can cause noticeable differences between the font sizes, if min and max are exaggerated. We can introduce an option to use the same min font size on every cell
  • we can also introduce the ability to add padding around the text to avoid having text touching each other along the edges.

Issues

fix #1295

Checklist

  • The proper chart type label has been added (e.g. :xy, :partition)
  • The proper feature labels have been added (e.g. :interactions, :axis)
  • All related issues have been linked (i.e. closes #123, fixes #123)
  • New public API exports have been added to packages/charts/src/index.ts
  • Unit tests have been added or updated to match the most common scenarios
  • The proper documentation and/or storybook story has been added or updated
  • The code has been checked for cross-browser compatibility (Chrome, Firefox, Safari, Edge)

@markov00
feat(heatmap): maximise the cell label fontSize
f98b548
@markov00
feat: change API to include min and max fontSize
b3d0a72
@markov00
fix: remove align/baseline to avoid unwanted configurations
eaf20e3
@markov00
docs: update api with changes on config
60c79cb 
BREAKING CHANGE: this commit and the previous ones introduce a breaking change in the configuration. The `fontSize`, `align` and `baseline` parameters are no longer available. Only `fontSize` is replaces by `minFontSize` and `maxFontSize`.
@markov00
fix: exported missing Pixels type
9bff0a6
@markov00
test: add vrts
c1eb040
@markov00 markov00 added bug Something isn't working :styling Styling related issue :heatmap Heatmap/Swimlane chart related issue labels Sep 2, 2021
monfera
monfera approved these changes Sep 3, 2021
View reviewed changes
Copy link
Contributor

@monfera monfera left a comment

Choose a reason for hiding this comment

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

LGTM 🎉 The three limitations mentioned in the description would be useful to solve in this or an eventual PR, depending on the priority landscape. Two of those items have visual impact and it may be better to offer the more defensible options first, and by default (allowing a slight padding, and allowing a capping of the font size a bit like what you asked for for the treemap back then). It's hard to wean users off of suboptimal defaults as they desensitize users for their weaknesses while accustom them for their (maybe lesser) benefits. But there may be reasons for expediency, or strong demand for the volatile font size

While the slight disentanglement of the union type is implementation detail, it's probably easier while the code is in working memory, and gain experience with a shift to a disambiguation layer. Maybe at some point it could be a listed PR eval criterion (no propagation of user facing union types into internals if it's reasonably solvable upstream)

nickofthyme
nickofthyme approved these changes Sep 3, 2021
View reviewed changes
Copy link
Collaborator

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

LGTM

darnautov
darnautov approved these changes Sep 6, 2021
View reviewed changes
Copy link
Contributor

@darnautov darnautov left a comment

Choose a reason for hiding this comment

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

LGTM

@markov00
Copy link
Member Author

markov00 commented Sep 6, 2021 •
edited
Loading

LGTM 🎉 The three limitations mentioned in the description would be useful to solve in this or an eventual PR, depending on the priority landscape. Two of those items have visual impact and it may be better to offer the more defensible options first, and by default (allowing a slight padding, and allowing a capping of the font size a bit like what you asked for for the treemap back then). It's hard to wean users off of suboptimal defaults as they desensitize users for their weaknesses while accustom them for their (maybe lesser) benefits. But there may be reasons for expediency, or strong demand for the volatile font size

@monfera please take a look at 9017878 and 3c2dc77 where I've added the text padding (it looks like a sensible default for now but happy to play with the value a bit more) and the default configuration that use the global minimum fontSize instead of a fontSize relative to each cell text value.

@markov00
Copy link
Member Author

markov00 commented Sep 6, 2021

@monfera I've seen @nickofthyme working on cleaning up the various interfaces/types/api and I prefer to wait him before adding a disambiguation layer for the fontSize at least

@markov00 markov00 mentioned this pull request Sep 7, 2021
Closed
@markov00 markov00 merged commit 16b5546 into elastic:master Sep 8, 2021
@markov00 markov00 deleted the 2021_09_02-resize_text_heatmap branch September 8, 2021 09:10
nickofthyme pushed a commit that referenced this pull request Sep 13, 2021
@semantic-release-bot
chore(release): 35.0.0 [skip ci]
b82f724 
# [35.0.0](v34.2.1...v35.0.0) (2021-09-13)

### Bug Fixes

* **a11y:** restore focus after popover close with color picker ([#1272](#1272)) ([0c6f945](0c6f945)), closes [#1266](#1266) [#935](#935)
* **build:** fix license in package.json ([#1362](#1362)) ([d524fdf](d524fdf))
* **deps:** update dependency @elastic/eui to ^37.5.0 ([#1341](#1341)) ([fb05c98](fb05c98))
* **deps:** update dependency @elastic/eui to ^37.6.1 ([#1359](#1359)) ([2ae90ce](2ae90ce))
* **deps:** update dependency @elastic/eui to ^37.7.0 ([#1373](#1373)) ([553b6b0](553b6b0))
* **heatmap:** filter out tooltip picked shapes in x-axis area ([#1351](#1351)) ([174047d](174047d)), closes [#1215](#1215)
* **heatmap:** remove values when brushing only over axes ([#1364](#1364)) ([77ff8d3](77ff8d3))

### Features

* **annotations:** add onClickHandler for annotations ([#1293](#1293)) ([48198be](48198be)), closes [#1211](#1211)
* **heatmap:** add text color contrast to heatmap cells ([#1342](#1342)) ([f9a26ef](f9a26ef)), closes [#1296](#1296)
* **heatmap:** reduce font size to fit label within cells ([#1352](#1352)) ([16b5546](16b5546))
* **xy:** mutilayer time axis step 1 ([#1326](#1326)) ([867b1f5](867b1f5))

### BREAKING CHANGES

* **xy:** - feat: removes the axis deduplication feature
- fix: `showDuplicatedTicks` causes a duplication check on the actual axis tick label (possibly yielded by `Axis.tickLabel` rather than the more general `tickFormat`)
* **heatmap:** the `config.label.fontSize` prop is replaced by `config.label.minFontSize` and `config.label.maxFontSize`. You can specify the same value for both properties to have a fixed font size. The `config.label.align` and `config.label.baseline` props are removed from the `HeatmapConfig` object.
@nickofthyme
Copy link
Collaborator

🎉 This PR is included in version 35.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@nickofthyme nickofthyme added the released Issue released publicly label Sep 13, 2021
@markov00 markov00 mentioned this pull request Oct 5, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monfera monfera monfera approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

@darnautov darnautov darnautov approved these changes

@rshen91 rshen91 Awaiting requested review from rshen91

Assignees
No one assigned
Labels
bug Something isn't working :heatmap Heatmap/Swimlane chart related issue released Issue released publicly :styling Styling related issue
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Heatmap] Values overflow cells overlapping other labels
4 participants
@markov00 @nickofthyme @monfera @darnautov