-
Notifications
You must be signed in to change notification settings - Fork 122
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): clip text in partition chart fill label #1033
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1033 +/- ##
==========================================
+ Coverage 72.84% 73.19% +0.35%
==========================================
Files 363 379 +16
Lines 11229 11544 +315
Branches 2442 2492 +50
==========================================
+ Hits 8180 8450 +270
- Misses 3035 3072 +37
- Partials 14 22 +8
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I've left a few minor comments
@@ -111,7 +111,7 @@ function getVerticalAlignment( | |||
case VerticalAlignments.bottom: | |||
return -(container.y1 - linePitch * (totalRowCount - 1 - rowIndex) - paddingBottom - fontSize * overhang); | |||
default: | |||
return -((container.y0 + container.y1) / 2 + (linePitch * (rowIndex - totalRowCount)) / 2); | |||
return -((container.y0 + container.y1) / 2 + (linePitch * (rowIndex + 1 - totalRowCount)) / 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:D
@@ -58,6 +66,7 @@ function renderTextRow( | |||
ctx.font = cssFontShorthand(box, fontSize); | |||
ctx.fillText(box.text, box.width / 2 + box.wordBeginning, 0); | |||
}); | |||
ctx.closePath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to call closePath
here, and probably we don't even need the ctx.beginPath();
at line 60 as fillText
doesn't need to instantiate a new path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed! The follow-up PR significantly restructures the Canvas render code, will explain reasons (motive: reduce context save/restore count)
}, | ||
}, | ||
}, | ||
clip: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we rename it clipText
within the code as it clips the text, not the geometry. I know that is part of the fillLabel
config, but we also have other attributes as textColor
textContrast
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# [25.1.0](v25.0.1...v25.1.0) (2021-03-01) ### Bug Fixes * rounding values on stacked w percentage charts ([#1039](#1039)) ([ee63a70](ee63a70)) ### Features * **axis:** log scale limit and base options ([#1032](#1032)) ([b38d110](b38d110)) * **partition:** clip text in partition chart fill label ([#1033](#1033)) ([be9bea0](be9bea0))
🎉 This PR is included in version 25.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [25.1.0](elastic/elastic-charts@v25.0.1...v25.1.0) (2021-03-01) ### Bug Fixes * rounding values on stacked w percentage charts ([opensearch-project#1039](elastic/elastic-charts#1039)) ([021e836](elastic/elastic-charts@021e836)) ### Features * **axis:** log scale limit and base options ([#1032](elastic/elastic-charts#1032)) ([11f94c6](elastic/elastic-charts@11f94c6)) * **partition:** clip text in partition chart fill label ([opensearch-project#1033](elastic/elastic-charts#1033)) ([7626441](elastic/elastic-charts@7626441))
Summary
Optionally clip text for fill label in partition chart, and fix an alignment issue.
Before:
After:

Checklist
Delete any items that are not applicable to this PR.
src/index.ts
(and stories only import from../src
except for test data & storybook)