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

Introduce horizontal colorbars #6024

Merged
merged 27 commits into from
Dec 10, 2021
Merged

Introduce horizontal colorbars #6024

merged 27 commits into from
Dec 10, 2021

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Nov 17, 2021

Resolves #1244 by adding orientation attribute to colorbar with the option 'h'.

@plotly/plotly_js

@nicolaskruchten
Copy link
Contributor

Looks good to me! Automargins on the right and left side don't work the way I would expect, but other than that it looks great!

@archmoj archmoj added this to the v2.7.0 milestone Nov 23, 2021
@alexcjohnson
Copy link
Collaborator

A couple of things I notice about how things are laid out:

Titles on the side: when the ticks & labels are on the inside it definitely seems like the title should be vertically centered. I think that also makes sense when the labels are outside (centered vertically wrt. the bar and labels together, not just the bar) but it's a little less clear to me in that case.
Screen Shot 2021-12-02 at 9 45 12 AM
Screen Shot 2021-12-02 at 9 47 32 AM

Outside labels on horizontal bars: first and last labels seem to intrude on the border padding. This is most evident with left/right ticklabelposition when the colorbar has a border and/or background color (as plenty of your excellent mocks do 🎉 ) but can also be seen with centered labels as in the second image above. I think without border & background the way it's drawn right now makes sense, as it would be unpleasant to have multiple colorbars that don't line up just because their tick labels have different lengths, but when you DO have a border or background color (different from paper_bgcolor) this situation should shrink the colorbar.
Screen Shot 2021-12-02 at 9 52 41 AM

@alexcjohnson
Copy link
Collaborator

Also, isn't there too much space below the colorbar when the title is on the side and the labels are below, as in the second screenshot in the comment above?

@archmoj archmoj modified the milestones: v2.7.0, v2.8.0 Dec 3, 2021
@archmoj
Copy link
Contributor Author

archmoj commented Dec 3, 2021

when you DO have a border or background color (different from paper_bgcolor) this situation should shrink the colorbar. Screen Shot 2021-12-02 at 9 52 41 AM

Is it only for horizontal colorbars or both orientations?

@archmoj
Copy link
Contributor Author

archmoj commented Dec 5, 2021

Titles on the side: when the ticks & labels are on the inside it definitely seems like the title should be vertically centered. I think that also makes sense when the labels are outside (centered vertically wrt. the bar and labels together, not just the bar) but it's a little less clear to me in that case. Screen Shot 2021-12-02 at 9 45 12 AM Screen Shot 2021-12-02 at 9 47 32 AM

This part was addressed in 8a137e3.

 - when there is a border line or different background
@archmoj
Copy link
Contributor Author

archmoj commented Dec 9, 2021

Outside labels on horizontal bars: first and last labels seem to intrude on the border padding. This is most evident with left/right ticklabelposition when the colorbar has a border and/or background color (as plenty of your excellent mocks do tada ) but can also be seen with centered labels as in the second image above. I think without border & background the way it's drawn right now makes sense, as it would be unpleasant to have multiple colorbars that don't line up just because their tick labels have different lengths, but when you DO have a border or background color (different from paper_bgcolor) this situation should shrink the colorbar. Screen Shot 2021-12-02 at 9 52 41 AM

This part is addressed in 2f09b6e by adjusting the position of first and last tick labels.

@archmoj archmoj requested a review from alexcjohnson December 9, 2021 17:30
@alexcjohnson
Copy link
Collaborator

How about we just remove these extremal ticks, in the cases you determine you need to shift them? It's an edge case already since mostly people won't be putting their colorbars in boxes, and the shifted labels are too easy to break (and even some non-broken cases don't look that great, when the number intersects the tick)
Screen Shot 2021-12-10 at 12 01 50 AM
Screen Shot 2021-12-10 at 12 02 37 AM
Screen Shot 2021-12-10 at 12 03 03 AM

Also, and this one I'm comfortable just making an issue for and deferring, outside left with many labels such that they auto-rotate does weird things:
Screen Shot 2021-12-10 at 12 10 55 AM
Screen Shot 2021-12-10 at 12 10 28 AM

This is an issue for regular cartesian axes too
Screen Shot 2021-12-10 at 12 15 54 AM

and for all shifted tick labels on both colorbars and cartesian axes if you provide explicit tickangle

@archmoj
Copy link
Contributor Author

archmoj commented Dec 10, 2021

How about we just remove these extremal ticks, in the cases you determine you need to shift them?

No I think it's better to keep them around and adjust their position.
Otherwise no labels would show up here:

h-colorbar_geo_multiple-usa-choropleths

@alexcjohnson
Copy link
Collaborator

OK, but it's way too easy to break and the breaking cases I noted above are just the tip of the iceberg - this would be a huge pain to fix. How about this then: as long as there will be at least 2 labels left, remove the ones that would need moving; if removing would result in 0 or 1 labels, we move them instead.

@alexcjohnson
Copy link
Collaborator

Titles on the side: when the ticks & labels are on the inside it definitely seems like the title should be vertically centered. I think that also makes sense when the labels are outside (centered vertically wrt. the bar and labels together, not just the bar) but it's a little less clear to me in that case. Screen Shot 2021-12-02 at 9 45 12 AM Screen Shot 2021-12-02 at 9 47 32 AM

This part was addressed in 8a137e3.

8a137e3 adjusted the horizontal alignment - I was talking about vertical

@archmoj
Copy link
Contributor Author

archmoj commented Dec 10, 2021

OK, but it's way too easy to break and the breaking cases I noted above are just the tip of the iceberg - this would be a huge pain to fix. How about this then: as long as there will be at least 2 labels left, remove the ones that would need moving; if removing would result in 0 or 1 labels, we move them instead.

Applied in 07cd536

@archmoj
Copy link
Contributor Author

archmoj commented Dec 10, 2021

Titles on the side: when the ticks & labels are on the inside it definitely seems like the title should be vertically centered. I think that also makes sense when the labels are outside (centered vertically wrt. the bar and labels together, not just the bar) but it's a little less clear to me in that case. Screen Shot 2021-12-02 at 9 45 12 AM Screen Shot 2021-12-02 at 9 47 32 AM

This part was addressed in 8a137e3.

8a137e3 adjusted the horizontal alignment - I was talking about vertical

The vertical alignment based on the height of colorbar title could be an option later on the road.
If one looks at the whole graph not only the colorbar, the current setup is pretty good IMHO.

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.

Excellent - let's add one or two mocks that exactly match existing mocks but with no border or bg for the colorbar, showing that the labels are all visible and unmoved. Then I'd say this is ready to merge and release! 💃

@archmoj
Copy link
Contributor Author

archmoj commented Dec 10, 2021

Excellent - let's add one or two mocks that exactly match existing mocks but with no border or bg for the colorbar, showing that the labels are all visible and unmoved. Then I'd say this is ready to merge and release! woman_dancing

Good call. Done in a0901bd.

@archmoj archmoj merged commit 5767a71 into master Dec 10, 2021
@archmoj archmoj deleted the h-colorbar branch December 10, 2021 21:02
@etpinard
Copy link
Contributor

Great feature!

Well done @archmoj !!

@archmoj
Copy link
Contributor Author

archmoj commented Dec 13, 2021

Great feature!

Well done @archmoj !!

Thanks for the review @etpinard :)

kMutagene added a commit to plotly/Plotly.NET that referenced this pull request Feb 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature something new
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow for horizontal colorbar
4 participants