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

feat(partition): linked text overflow avoidance #670

Merged

Conversation

monfera
Copy link
Contributor

@monfera monfera commented May 7, 2020

Summary

Truncates linked label text to fit in the chart box (WIP)

Example with off-center pie chart to avoid centered special case:
image

It culls a link label altogether if

  • its text has no horizontal room left
  • the text would overflow the rectangular area vertically; eg. no linked label for the green Food... slice:
    image

Closes #633

Checklist

Delete any items that are not applicable to this PR.

  • Any consumer-facing exports were added to src/index.ts (and stories only import from ../src except for test data & storybook)
  • This was checked for cross-browser compatibility, including a check against IE11
  • Proper documentation or storybook story was added for features that require explanation or tutorials
  • Unit tests were updated or added to match the most common scenarios

@monfera monfera added enhancement New feature or request wip work in progress :partition Partition/PieChart/Donut/Sunburst/Treemap chart related labels May 7, 2020
@monfera monfera self-assigned this May 7, 2020
@monfera monfera removed the wip work in progress label May 11, 2020
@monfera
Copy link
Contributor Author

monfera commented May 11, 2020

Removed the wip label as it has the vertical space feature as well.
The image tests capture the desired effect to a greater extent than unit tests could. I also have plans to revisit it for improvements, eg. more accurate maximalization of horizontal space.

The unit testing plan for a subsequent PR is the following:

  • extract out and maybe rename the maximizer function which is basically a greedy search, gradient descent function
  • switch to this function for the determination of font sizes for fill text, which currently iterates through the font sizes linearly, and not yet modular anyway
  • the optimizer thus becomes exported, add unit tests for it

@codecov-io
Copy link

Codecov Report

Merging #670 into master will decrease coverage by 0.37%.
The diff coverage is 6.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #670      +/-   ##
==========================================
- Coverage   72.97%   72.60%   -0.38%     
==========================================
  Files         266      266              
  Lines        8634     8678      +44     
  Branches     1698     1710      +12     
==========================================
  Hits         6301     6301              
- Misses       2294     2338      +44     
  Partials       39       39              
Impacted Files Coverage Δ
...hart_types/partition_chart/layout/config/config.ts 68.75% <ø> (ø)
...types/partition_chart/layout/types/config_types.ts 100.00% <ø> (ø)
...es/partition_chart/layout/types/viewmodel_types.ts 80.00% <ø> (ø)
...rtition_chart/layout/viewmodel/link_text_layout.ts 10.12% <0.00%> (-8.93%) ⬇️
src/utils/data_generators/simple_noise.ts 100.00% <ø> (ø)
src/utils/data_generators/data_generator.ts 44.44% <22.22%> (-10.73%) ⬇️
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 89.23% <100.00%> (ø)
src/mocks/utils.ts 94.11% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a79e899...2f21442. Read the comment docs.

Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

LGTM, I've added a comment to include the TSDoc for the monotonicMaximizer function as it's a little bit difficult to understand looking just at the function signature.

.filter((l: LinkLabelVM) => l.text !== ''); // cull linked labels whose text was truncated to nothing
}

function monotonicMaximizer(
Copy link
Member

Choose a reason for hiding this comment

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

nit: Could you please add a jsdoc describing what this function does?
It's a little cryptic and I've difficulties understanding what is going on there

Copy link
Member

Choose a reason for hiding this comment

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

Please, also add the return type

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Marco, will do, the upcoming PR deals with and extracts out this function

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.

Code and functionality all LGTM

@codecov-commenter
Copy link

Codecov Report

Merging #670 into master will decrease coverage by 0.37%.
The diff coverage is 6.89%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #670      +/-   ##
==========================================
- Coverage   72.97%   72.60%   -0.38%     
==========================================
  Files         266      266              
  Lines        8634     8678      +44     
  Branches     1698     1710      +12     
==========================================
  Hits         6301     6301              
- Misses       2294     2338      +44     
  Partials       39       39              
Impacted Files Coverage Δ
...hart_types/partition_chart/layout/config/config.ts 68.75% <ø> (ø)
...types/partition_chart/layout/types/config_types.ts 100.00% <ø> (ø)
...es/partition_chart/layout/types/viewmodel_types.ts 80.00% <ø> (ø)
...rtition_chart/layout/viewmodel/link_text_layout.ts 10.12% <0.00%> (-8.93%) ⬇️
src/utils/data_generators/simple_noise.ts 100.00% <ø> (ø)
src/utils/data_generators/data_generator.ts 44.44% <22.22%> (-10.73%) ⬇️
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 89.23% <100.00%> (ø)
src/mocks/utils.ts 94.11% <100.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a79e899...2f21442. Read the comment docs.

@monfera monfera merged commit b6e5911 into elastic:master May 19, 2020
@monfera monfera deleted the partition-linked-text-overflow-squashed branch May 19, 2020 19:50
markov00 pushed a commit that referenced this pull request May 28, 2020
# [19.4.0](v19.3.0...v19.4.0) (2020-05-28)

### Bug Fixes

* **partition:** consider legendMaxDepth on legend size ([#654](#654)) ([9429dcf](9429dcf)), closes [#639](#639)

### Features

* **partition:** enable grooves in all group layers ([#666](#666)) ([f5b4767](f5b4767))
* **partition:** linked text overflow avoidance ([#670](#670)) ([b6e5911](b6e5911)), closes [#633](#633)
* **partition:** monotonic font size scaling ([#681](#681)) ([ea2489b](ea2489b)), closes [#661](#661)
* **tooltip:** improve positioning with popperjs ([#651](#651)) ([6512950](6512950)), closes [#596](#596)
@markov00
Copy link
Member

🎉 This PR is included in version 19.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label May 28, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request :partition Partition/PieChart/Donut/Sunburst/Treemap chart related released Issue released publicly
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pie chart linked labels are being displayed in cut off way
5 participants