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

Bar chart with second y axis overlaps data series #4150

Merged
merged 4 commits into from
Aug 5, 2020
Merged

Bar chart with second y axis overlaps data series #4150

merged 4 commits into from
Aug 5, 2020

Conversation

kravets-levko
Copy link
Collaborator

@kravets-levko kravets-levko commented Sep 18, 2019 •
edited
Loading

What type of PR is this? (check all applicable)

  • Bug Fix

Description

Assign offsetgroup for bar series so Plotly will offset them properly.

Related Tickets & Documents

Fixes #2292

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Before:
image

After (the same chart settings and data, but with offsetgroup):
image

@kravets-levko kravets-levko self-assigned this Sep 18, 2019
@kravets-levko kravets-levko marked this pull request as ready for review September 18, 2019 13:45
ranbena
ranbena reviewed Sep 18, 2019
View reviewed changes
Copy link
Contributor

@ranbena ranbena left a comment

Choose a reason for hiding this comment

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

Noticed in demo that some of the widget visualizations changed.
For instance, Group Scatter (x axis labels hidden), Labels (same), Basic Heatmap (blocks now distinguishable which is good?), Basic Bubble (axis offset).

@ranbena
Copy link
Contributor

ranbena commented Sep 18, 2019

+556 −1,708 😲😛

@kravets-levko kravets-levko changed the title Bar chart with second y axis overlaps data series (WIP) Bar chart with second y axis overlaps data series Sep 18, 2019
@kravets-levko
Copy link
Collaborator Author

Noticed in demo that some of the widget visualizations changed.

Yeah, I suppose it's because of plotly.js upgrade. Need to test it properly before merging.

+556 −1,708

That's lockfile

@kravets-levko
Copy link
Collaborator Author

Group Scatter (x axis labels hidden), Labels (same)

I see x-axis labels there:

image
image

Bubble axis offset - yeah, that's quite interesting, considering that on both https://deploy-preview-4150--redash-preview.netlify.com/dashboard/demo and https://redash-preview.netlify.com/dashboard/demo it looks wrong 🤔

image
image

Heatmap difference may be related to my recent refactor, I'll check.

Also, seems there is a very subtle difference in plot inner paddings - they are a bit larger on this branch 🤔

@ranbena
Copy link
Contributor

ranbena commented Sep 19, 2019

I think it has to do with my screen resolution (width 1400px) so it now looks like this:

Screen Shot 2019-09-19 at 10 49 51

Screen Shot 2019-09-19 at 10 49 23

Also, seems there is a very subtle difference in plot inner paddings

I noticed it but don't mind it. Not a dramatic change.

@weekly-digest weekly-digest bot mentioned this pull request Sep 23, 2019
Closed
@kravets-levko kravets-levko mentioned this pull request Nov 27, 2019
Closed
@kravets-levko kravets-levko mentioned this pull request Jan 2, 2020
Closed
@kravets-levko kravets-levko mentioned this pull request Mar 30, 2020
Merged
1 task
@kravets-levko
Copy link
Collaborator Author

@gabrieldutra @arikfr (@ranbena if you're still interested in this 😄) I updated this PR and now it contains only changes related to the bug it's fixing (no more Plotly upgrade - we did it separately). I kindly ask you to review it once more 🙇‍♂️

@arikfr If v9 will contain upgraded Plotly - maybe it makes some sense to include this fix as well? WDYT?

@kravets-levko kravets-levko changed the title (WIP) Bar chart with second y axis overlaps data series Bar chart with second y axis overlaps data series Apr 7, 2020
@orenpai
Copy link

orenpai commented May 5, 2020 •
edited
Loading

+1 for pulling this one in, I'm affected by this issue too. Thanks for your contribution!

@weekly-digest weekly-digest bot mentioned this pull request May 11, 2020
Closed
@kravets-levko kravets-levko mentioned this pull request Aug 3, 2020
Closed
@kravets-levko kravets-levko merged commit eb603f6 into master Aug 5, 2020
@kravets-levko kravets-levko deleted the fix/bar-chart-2292 branch August 5, 2020 17:28
andrewdever pushed a commit to andrewdever/redash that referenced this pull request Oct 5, 2020
@kravets-levko @andrewdever
Bar chart with second y axis overlaps data series (getredash#4150)
6c6fa5b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ranbena ranbena ranbena left review comments

@gabrieldutra gabrieldutra gabrieldutra approved these changes

@arikfr arikfr Awaiting requested review from arikfr

Assignees

@kravets-levko kravets-levko

Labels
Frontend Visualizations: Chart Visualizations
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bar chart with second y axis overlaps data series
4 participants
@kravets-levko @ranbena @orenpai @gabrieldutra