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.

Sign up for GitHub

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

Jump to bottom

Make sunburst and treemap sort optional #5164

Merged
merged 4 commits into from
Sep 28, 2020
Merged

Make sunburst and treemap sort optional #5164

merged 4 commits into from
Sep 28, 2020

Conversation

thierryVergult
Copy link
Contributor

  • Standard (original) behavior: sort on values
  • new sort option to disable sorting

see: Disable automatic sorting of the slices in the sunburst #4823

@thierryVergult
sunburst sort attribute
61c4a83 
- Standard (original) behavior: sort on values
- new *sort* option to disable sorting
@archmoj archmoj added status: in progress community community contribution feature something new labels Sep 24, 2020
@thierryVergult
reuse pie.sort
0a531f8 
reuse pie.sort in attributes, default true
@thierryVergult
Copy link
Contributor Author

I will try to figure out why these 3 lines of code resulted in 4 failing checks

@thierryVergult
fix indentation & space
cf66d2e 
satisfy eslint
@archmoj
Copy link
Contributor

archmoj commented Sep 26, 2020

I will try to figure out why these 3 lines of code resulted in 4 failing checks

Please also add an identical attribute to treemap trace.

@archmoj archmoj changed the title sunburst sort attribute Make sunburst and treemap sort optional Sep 26, 2020
@thierryVergult
Copy link
Contributor Author

May I ask why the sort attribute should be added to th treemap? Should that resolve some of the failing tests? Or should that make the solution more feature complete? thanks.

@archmoj
Copy link
Contributor

archmoj commented Sep 26, 2020

May I ask why the sort attribute should be added to th treemap? Should that resolve some of the failing tests? Or should that make the solution more feature complete? thanks.

Sure. Actually both.

@archmoj
Copy link
Contributor

archmoj commented Sep 26, 2020

Please also note that treemap uses sunburst calc. So right now (without a treemap.sort option) on your branch, treemap does not sort.

@thierryVergult
treemap sort
a66b42d 
- obey npm lint
- add sort option to treemap (define attribute & coerce in default

Treepad is using the calc from sunburst, so the 1 line code change for  sunburst impacted treemap (sort was not true)
@archmoj
Copy link
Contributor

archmoj commented Sep 27, 2020

Looking good to me. 💃
@alexcjohnson @nicolaskruchten please merge if there is no problem in terms of the API.
Then I could add/adjust one image test for this feature as well as minexponent feature added in #5121 together.

@archmoj archmoj added this to the v1.56.0 milestone Sep 27, 2020
alexcjohnson
alexcjohnson approved these changes Sep 28, 2020
View reviewed changes
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks @thierryVergult! @archmoj go ahead and merge once the tests pass (looks like just a flaky failure, I reran)

@archmoj archmoj merged commit d191ac4 into plotly:master Sep 28, 2020
This was referenced Sep 28, 2020
Closed
Merged
@dmt0 dmt0 mentioned this pull request Sep 30, 2020
Open
@thierryVergult thierryVergult mentioned this pull request Dec 28, 2020
Closed
@Skrattoune Skrattoune mentioned this pull request Mar 17, 2023
Closed
@thierryVergult thierryVergult deleted the sunburstSort branch June 1, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@archmoj archmoj archmoj left review comments

@alexcjohnson alexcjohnson alexcjohnson approved these changes

Assignees
No one assigned
Labels
community community contribution feature something new
Projects
None yet
Milestone
v1.56.0
Development

Successfully merging this pull request may close these issues.
3 participants
@thierryVergult @archmoj @alexcjohnson