-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
combo stacked chart #42085
combo stacked chart #42085
Conversation
3a8491e
to
08acdf3
Compare
|
9f4ab23
to
5c3e1f7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
5e44bdf
to
00611b6
Compare
Nice! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
99% LGTM, but there are still a few bugs that make this kind of hard to use. Perhaps these issues are already being tracked separately.
2024-05-07.17-15-23.mp4
When bars are stuck under an area, you cannot hover over the bars and see their tool-tip values. Also, when areas are not stacked, hovering over the smaller one will highlight the larger one (when it should be highlighting the smaller one).
@JesseSDevaney thanks for the review! I created an issue to track it #42366 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sasha! I found a couple more bugs upon further investigation.
- The ability to select the y-axis position for stacked charts is broken (comment)
- With the new ability of combo charts, we probably still want to show the
Show values on data points
setting if we have a line series in a stacked 100% chart (comment)
Once we get these bugs fixed up and fix the type errors + unit test failures (probably coming from updates to the number of arguments in a function and how we handle the data models), we should be good to merge.
...hrome_laptop_static_viz_ComboChart_Bar_Breakout_With_Line_Series_Stacked_Right_Axis_Only.png
Outdated
Show resolved
Hide resolved
.loki/reference/chrome_laptop_static_viz_ComboChart_Combo_Stacked_Bars_Areas_Normalized.png
Outdated
Show resolved
Hide resolved
frontend/src/metabase/static-viz/components/ComboChart/ComboChart.stories.tsx
Show resolved
Hide resolved
1ebd298
to
c233fd8
Compare
@alxnddr In my PR to move data label formatters from See #42616 (comment) for more context |
* propagate combo stacking to area and bar charts * enable combo stacking for bar/area charts, fix data transform * fix types, specs * fix y-axis extents calculations * remove spec that tested the removed control, update specs, update test data
c0866d9
to
3ef60e9
Compare
}, | ||
); | ||
|
||
expect(isHidden).toBe(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect(isHidden).toBe(true); | |
expect(isHidden).toBe(false); |
Should be visible implies isHidden = false
?
frontend/src/metabase/visualizations/lib/settings/graph.unit.spec.js
Outdated
Show resolved
Hide resolved
@JesseSDevaney good catch, thanks, I fixed it |
Closes #41976
Closes #12008
Description
Display
tab, add a new radio setting:Bar and area stacking
Don't stack
Stack
Stack - 100%
Stack
orStack - 100%
is selected then:Display type
set tobar
should be stacked together.Display type
set toarea
should be stacked together.Display type
set toline
should be unaffected.Stack - 100%
is chosen, if there are any series set toline
, there should be two y-axes:How to verify
Demo
Checklist