Skip to content

Revise hierarchy referencing and fixup parent percentage at the root node of treemap and sunburst - #4219

Merged
archmoj merged 5 commits into
masterfrom
hierarchy-percentages
Sep 30, 2019
Merged

Revise hierarchy referencing and fixup parent percentage at the root node of treemap and sunburst #4219
archmoj merged 5 commits into
masterfrom
hierarchy-percentages

Conversation

@archmoj

@archmojarchmoj commented Sep 26, 2019

Copy link
Copy Markdown
Contributor

This PR

  • simplifies the plot and hover code for displaying percentages on sunburst and treemap plots
  • fixes a bug at the root node | now the hover/text template/info are available there
  • adds jasmine tests for treemap hovertemplate at different levels
  • uploads the CI version of a baseline which started failing after we merged Treemap new trace type #4185
    @etpinard

@archmoj
Correct percentages at root
7a1c0e5
- simplify sunburst and treemap percentage references
- add hovertemplate tests to treemap
- fixup sunburst jasmine texttemplate tests
@archmoj
copy one of the treemap baselines from the CI which started failing t…
422d5d3
…here after merging the treemap PR
@archmojarchmoj added this to the v1.50.0 milestone Sep 26, 2019
@archmojarchmoj changed the title Revise hierarchy parent percentage at the root node of treemap and sunburst Revise hierarchy referencing and fixup parent percentage at the root node of treemap and sunburst Sep 26, 2019
etpinard
etpinard reviewed Sep 26, 2019
View reviewed changes
Comment threadsrc/traces/sunburst/helpers.js Outdated
etpinard
etpinard reviewed Sep 26, 2019
View reviewed changes
Comment threadtest/jasmine/tests/sunburst_test.js Outdated
['%{percentRoot} of %{root}', ['100% of Eve', '33% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve']],
['%{percentEntry} of %{entry}', ['100% of Eve', '33% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve']],
['%{percentParent} of %{parent}', ['%{percentParent} of %{parent}', '100% of Seth', '33% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '50% of Seth', '100% of Awan']],
['%{percentParent} of %{parent}', ['100% of Eve', '100% of Seth', '33% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '17% of Eve', '50% of Seth', '100% of Awan']],

@etpinardetpinardSep 26, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Wait. Is this the root node sector? You're saying the parent of the root node is itself?

@archmojarchmojSep 28, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Good call. Fixed in 54360be.

@etpinardetpinardSep 30, 2019•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't get it. After 54360be, looks like %{parent} is now '', but %{percentParent} is still 100% for the root node.

Can we have '%{percentParent} of %{parent}' resolve as '%{percentParent} of %{parent}' for the root node (i.e. the percentParent and parent keys are not defined) like before?

I know this is not ideal, but I don't like filling in '%{percentParent} and %{parent} with wrong values for the root node just to make the template string look better.

@archmojarchmojSep 30, 2019

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Done in 7155917.

etpinard
etpinard reviewed Sep 26, 2019
View reviewed changes
Comment threadtest/jasmine/tests/treemap_test.js Outdated
@etpinardetpinard mentioned this pull request Sep 27, 2019
Merged
@etpinardetpinard mentioned this pull request Sep 30, 2019
Merged
archmoj
archmoj commented Sep 30, 2019
View reviewed changes
Comment threadsrc/traces/sunburst/calc.js
@etpinard

etpinard commented Sep 30, 2019

Copy link
Copy Markdown
Contributor

Nicely done 💃 thanks!!

@archmoj
archmoj merged commit 5673cd1 into masterSep 30, 2019
@archmoj
archmoj deleted the hierarchy-percentages branch September 30, 2019 17:30
@snyk-botsnyk-bot mentioned this pull request Feb 4, 2020
Merged
@snyk-botsnyk-bot mentioned this pull request Feb 22, 2020
Open
@snyk-botsnyk-bot mentioned this pull request Apr 17, 2020
Open
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@etpinardetpinardetpinard left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

bugsomething broken

Projects

None yet

Milestone

v1.50.0

Development

Successfully merging this pull request may close these issues.

2 participants

@archmoj@etpinard