Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Do not assign offsetgroup to traces in px unless barmode="group" #4865

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

Merged
emilykl merged 17 commits into master from fix-bar-charts
Nov 18, 2024

Conversation

@emilykl
Copy link
Contributor

@emilykl emilykl commented Nov 12, 2024
edited
Loading

Closes #4863

This change is required due to the change in behavior for offsetgroup introduced in Plotly.js 3.0: plotly/plotly.js#7009

The above change causes bars to appear as a group if offsetgroup is set, even if barmode is set to stacked, which changes the default behavior of histograms and bar charts, as explained in #4863 (see comment).

This PR fixes the issue by only setting offsetgroup if barmode="group". (We also set offsetgroup if barmode is not set, since group is the default barmode in Plotly.js.)

Note that we can't remove setting offsetgroup entirely due to the issue described in my comment here.

The other change (changing the point at which we set barmode to overlay if the figure has marginal histograms) is so that the figure's barmode in that case will be set before the point where we decide whether to set offsetgroup.

MarcoGorelli reacted with heart emoji
Copy link
Contributor Author

emilykl commented Nov 12, 2024
edited
Loading

Unfortunately, not using offsetgroup at all breaks an edge case for faceted charts, as demonstrated by the percy tests histogram_histfunc and bar_facet.

Without offsetgroup, when there is no data in a facet for one bar in a group of bars, the other bars expand to fill the available space.

See comparison below — I believe the behavior on master is the way that this chart should be rendered.

master fix-bar-charts (this branch)
Screen Shot 2024年11月12日 at 1 34 44 PM Screen Shot 2024年11月12日 at 1 34 55 PM
Screen Shot 2024年11月12日 at 1 42 17 PM Screen Shot 2024年11月12日 at 1 40 45 PM

@gvwilson gvwilson added P1 needed for current cycle fix fixes something broken labels Nov 13, 2024
@emilykl emilykl changed the title (削除) Do not assign offsetgroup to traces in px for bar, violin, box, and histogram traces (削除ここまで) (追記) Do not assign offsetgroup to traces in px unless barmode="grouped" (追記ここまで) Nov 14, 2024
@emilykl emilykl changed the title (削除) Do not assign offsetgroup to traces in px unless barmode="grouped" (削除ここまで) (追記) Do not assign offsetgroup to traces in px unless barmode="group" (追記ここまで) Nov 14, 2024
@emilykl emilykl marked this pull request as ready for review November 14, 2024 22:34
Copy link
Contributor Author

emilykl commented Nov 14, 2024

Note: The 4 visual changes in Percy are the 4 charts which were incorrect on master and are now returning to how they should look.

Copy link
Contributor

thanks @emilykl - @marthacryan please review

Copy link
Collaborator

@marthacryan marthacryan left a comment

Choose a reason for hiding this comment

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

LGTM

emilykl reacted with rocket emoji
@emilykl emilykl merged commit 84f4e66 into master Nov 18, 2024
4 of 5 checks passed
@emilykl emilykl deleted the fix-bar-charts branch November 18, 2024 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marthacryan marthacryan marthacryan approved these changes

@LiamConnors LiamConnors Awaiting requested review from LiamConnors

@archmoj archmoj Awaiting requested review from archmoj

@gvwilson gvwilson Awaiting requested review from gvwilson

Labels

fix fixes something broken P1 needed for current cycle

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Some charts rendering differently on master branch vs Plotly.py 5.24.1

AltStyle によって変換されたページ (->オリジナル) /