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 maximum length config #665

Merged
merged 3 commits into from
May 7, 2020

Conversation

monfera
Copy link
Contributor

@monfera monfera commented May 5, 2020

Summary

Allows a configuration of maximum text length (in characters) for linked labels on pie and donut charts

image

Mitigates #633
Ticks a checkbox in #518

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 <- in follow-up PR for adaptive length constraints

@monfera monfera added the :partition Partition/PieChart/Donut/Sunburst/Treemap chart related label May 5, 2020
@monfera monfera requested a review from markov00 May 5, 2020 16:36
@monfera monfera self-assigned this May 5, 2020
@codecov-io
Copy link

codecov-io commented May 5, 2020

Codecov Report

Merging #665 into master will increase coverage by 0.03%.
The diff coverage is 89.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   72.94%   72.97%   +0.03%     
==========================================
  Files         266      266              
  Lines        8618     8634      +16     
  Branches     1695     1698       +3     
==========================================
+ Hits         6286     6301      +15     
- Misses       2293     2294       +1     
  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 19.04% <0.00%> (-0.47%) ⬇️
...artition_chart/renderer/canvas/canvas_renderers.ts 6.20% <0.00%> (ø)
...rtition_chart/layout/viewmodel/fill_text_layout.ts 95.31% <100.00%> (+0.36%) ⬆️
...tion_chart/layout/viewmodel/hierarchy_of_arrays.ts 88.88% <100.00%> (ø)
...ypes/partition_chart/layout/viewmodel/viewmodel.ts 89.23% <100.00%> (+0.08%) ⬆️

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 4421748...11820e5. Read the comment docs.

@monfera monfera added the enhancement New feature or request label May 5, 2020
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, left a small side note

@@ -69,7 +70,8 @@ export function linkTextLayout(
const stemFromY = y;
const stemToX = x + north * west * cy - west * relativeY;
const stemToY = cy;
const text = rawTextGetter(node);
const rawText = rawTextGetter(node);
const text = rawText.length <= maxTextLength ? rawText : `${rawText.substr(0, maxTextLength - 1)}…`; // ellipsis is one char
Copy link
Member

Choose a reason for hiding this comment

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

it's fine for now, in the future we should account for RTL languages :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, thanks for the nudge, will incorporate in the follow-up.

@monfera monfera merged commit 7166e42 into elastic:master May 7, 2020
@monfera monfera deleted the partition-linked-text-max-length branch May 7, 2020 17:43
markov00 pushed a commit that referenced this pull request May 8, 2020
# [19.3.0](v19.2.0...v19.3.0) (2020-05-08)

### Bug Fixes

* build/type issue with DataGenerator ([#671](#671)) ([86dd2b1](86dd2b1))

### Features

* **partition:** linked text maximum length config ([#665](#665)) ([7166e42](7166e42))
@markov00
Copy link
Member

markov00 commented May 8, 2020

🎉 This PR is included in version 19.3.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@markov00 markov00 added the released Issue released publicly label May 8, 2020
AMoo-Miki pushed a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Feb 10, 2022
# [19.3.0](elastic/elastic-charts@v19.2.0...v19.3.0) (2020-05-08)

### Bug Fixes

* build/type issue with DataGenerator ([opensearch-project#671](elastic/elastic-charts#671)) ([01844ac](elastic/elastic-charts@01844ac))

### Features

* **partition:** linked text maximum length config ([opensearch-project#665](elastic/elastic-charts#665)) ([e37cb8f](elastic/elastic-charts@e37cb8f))
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.

3 participants