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

horizontal legend width should be layout width #5108

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

p1-ra
Copy link

@p1-ra p1-ra commented Aug 29, 2020

Currently, horizontal legend width is set to the width of its longer text. It make the display of the scrollbar a little bit disturbing,

Capture_hlegend_broken

This PR fix horizontal legend width and set it to the full layout width:

Capture_hlegend_fixed

@archmoj
Copy link
Contributor

archmoj commented Sep 1, 2020

Thanks for the PR.
Seems like a good catch at first glance.
However there are a number of baselines that change.
So maybe one could consider enabling this option as a feature.

For now, could you please regenerate these?

geo_choropleth-legend
gl2d_fill-ordering
gl2d_parcoords_blocks
gl3d_mesh3d_coloring
legend_horizontal
legend_horizontal_wrap-alll-lines
legend_negative_x
legend_negative_x2
legend_scroll_beyond_plotarea
legendgroup_horizontal_bg_fit
pie_automargin
uniformtext_bar-like_10_auto
uniformtext_bar-like_8_textangle
uniformtext_bar-like_8_textangle45

Also please visit https://github.com/plotly/plotly.js/blob/master/CONTRIBUTING.md#image-pixel-comparison-tests for more info regarding generating the baselines.

@@ -742,6 +742,9 @@ function computeLegendDimensions(gd, groups, traces, opts) {
opts._titleWidth + 2 * (bw + constants.titlePad)
)
);
if (!isVertical) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To pass the syntax test,
no space required after if statement.

if(!isVertical) {

Also you could run syntax test locally using:

npm run test-syntax

@alexcjohnson
Copy link
Collaborator

Interesting, I certainly see the appeal of this, but I feel like it should at least be scoped to legends that have a scrollbar (the difference becomes important if the legend has a background or border), and possibly (as @archmoj mentioned) be turned into an opt-in attribute like legend.width

@archmoj archmoj added status: in progress community community contribution feature something new labels Sep 23, 2020
@nicolaskruchten
Copy link
Contributor

A small update on timing: our team is working hard on releasing v2.0 of Plotly.js, which we anticipate will happen in early April. This PR would be a good candidate to land in the library in v2.1 or later, so with apologies for the delay, we will likely not be able to give much feedback on this PR for the next few weeks :)

@gvwilson gvwilson self-assigned this Jun 10, 2024
@gvwilson
Copy link
Contributor

This pull request has been sitting for a while, so I would like to close it as part of our effort to tidy up our public repositories. I've assigned it to myself to keep track of it; I'll wait until 2024-06-17 for someone to say it's still relevant and they'll to take it on, and otherwise I will close it then. Thanks - @gvwilson

@gvwilson gvwilson removed their assignment Aug 2, 2024
@gvwilson gvwilson changed the title Fix: horizontal legend width should be layout width horizontal legend width should be layout width Aug 8, 2024
@gvwilson gvwilson added fix fixes something broken P2 considered for next cycle and removed feature something new labels Aug 8, 2024
@gvwilson gvwilson requested a review from marthacryan August 21, 2024 15:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution fix fixes something broken P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants