-
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
fix(treemap): align onElementClick parameters to sunburst #636
fix(treemap): align onElementClick parameters to sunburst #636
Conversation
This commit align the shape of passed parameter of the onElementClick listener to the one passed by a sunburst. For each single hovered shape (usually only one shape at time) it returns the values for each layer of the treemap fix elastic#624
Codecov Report
@@ Coverage Diff @@
## master #636 +/- ##
==========================================
+ Coverage 66.44% 74.34% +7.90%
==========================================
Files 240 252 +12
Lines 7012 7192 +180
Branches 1340 1360 +20
==========================================
+ Hits 4659 5347 +688
+ Misses 2337 1821 -516
- Partials 16 24 +8
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, great test too, thanks!
@@ -308,3 +290,23 @@ export function shapeViewModel( | |||
outerRadius, | |||
}; | |||
} | |||
|
|||
function partToShapeTreeNode(treemapLayout: boolean, innerRadius: Radius, ringThickness: number) { |
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.
Nice extraction! Should it be placed above the line in which we use it?
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've extracted that because I was having a hard time understanding the code.
My Place them the above the line that uses it, do you mean moving that function declaration before const quadViewModel = makeQuadViewModel(
?
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 think Robert is referring to preventing the function was used before it was declared
error. Basically best practice even though hoisting prevents any errors.
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, verified issue is resolved. Tested 4_sunburst_slice_clicks
story with changes.
# [18.4.0](v18.3.0...v18.4.0) (2020-04-22) ### Bug Fixes * **partition:** single slice wrong text positioning ([#643](#643)) ([6298d36](6298d36)), closes [#637](#637) * **treemap:** align onElementClick parameters to sunburst ([#636](#636)) ([2c1d224](2c1d224)), closes [#624](#624) ### Features * allow colorVariant option for series specific color styles ([#630](#630)) ([e5a206d](e5a206d)) * **series:** BubbleSeries (alpha) and markSizeAccessor ([#559](#559)) ([3aa235e](3aa235e))
🎉 This PR is included in version 18.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
# [18.4.0](elastic/elastic-charts@v18.3.0...v18.4.0) (2020-04-22) ### Bug Fixes * **partition:** single slice wrong text positioning ([opensearch-project#643](elastic/elastic-charts#643)) ([f8b5b8a](elastic/elastic-charts@f8b5b8a)), closes [opensearch-project#637](elastic/elastic-charts#637) * **treemap:** align onElementClick parameters to sunburst ([opensearch-project#636](elastic/elastic-charts#636)) ([8dd87bf](elastic/elastic-charts@8dd87bf)), closes [opensearch-project#624](elastic/elastic-charts#624) ### Features * allow colorVariant option for series specific color styles ([opensearch-project#630](elastic/elastic-charts#630)) ([e2444ef](elastic/elastic-charts@e2444ef)) * **series:** BubbleSeries (alpha) and markSizeAccessor ([opensearch-project#559](elastic/elastic-charts#559)) ([85d9bda](elastic/elastic-charts@85d9bda))
Summary
This commit aligns the shape of the passed parameter of the onElementClick listener to the one passed by a sunburst. For each single hovered shape (usually only one shape at a time) it returns the values for each layer of the treemap
fix #624
Checklist