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

propagate combo stacking to area and bar charts #42600

Merged
merged 5 commits into from
May 14, 2024

Conversation

alxnddr
Copy link
Member

@alxnddr alxnddr commented May 13, 2024

Epic #41976

Description

This PR enables creating stacked combo charts for Bar/Area visualizations. It requires removing Stacked chart type which allowed to override area/bar chart type:
Screenshot 2024-05-13 at 10 51 23 PM

It was a convenient way to switch between area/bar appearance although redundant. Now this setting does not make sense since it is possible to have both area and bar stacks on the same chart.

This PR also fixes the issue on non-linear y-axis scales when some series were stacked and others don't.

How to verify

  • Create multiseries stacked bar/area charts
  • Change series settings to create mixed area/bar/line charts with stacking
  • Ensure it works for all y-axis scale types

Demo

Screenshot 2024-05-13 at 10 53 09 PM

Checklist

  • Tests have been added/updated to cover changes in this PR

@metabase-bot metabase-bot bot added the .Team/DashViz Dashboard and Viz team label May 13, 2024
@alxnddr alxnddr requested a review from a team May 14, 2024 01:56
@alxnddr alxnddr marked this pull request as ready for review May 14, 2024 01:56
@metabase-bot metabase-bot bot added the visual Run Percy visual testing label May 14, 2024
@alxnddr alxnddr requested a review from cdeweyx May 14, 2024 01:58
@alxnddr alxnddr force-pushed the propagate-combo-stacking-to-areas-bars branch from e17fa52 to a992419 Compare May 14, 2024 02:01
Copy link

replay-io bot commented May 14, 2024

Status Complete ↗︎
Commit bcf5145
Results
⚠️ 2 Flaky
2506 Passed

@alxnddr alxnddr force-pushed the propagate-combo-stacking-to-areas-bars branch from e812eaa to f23eb15 Compare May 14, 2024 05:10
@alxnddr alxnddr force-pushed the propagate-combo-stacking-to-areas-bars branch from f23eb15 to 5a3ae84 Compare May 14, 2024 07:18
Copy link

@cdeweyx cdeweyx left a comment

Choose a reason for hiding this comment

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

🙌

@EmmadUsmani EmmadUsmani self-requested a review May 14, 2024 17:55
@@ -363,13 +368,15 @@ function getStackedValueTransformFunction(
}, 0);
const rawTotal = rawBelowTotal + getNumberOrZero(datum[seriesDataKey]);

const transformedRawBelowTotal = valueTransform(rawBelowTotal) ?? 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be better on line 377 so it's underneath the comment explaining that step.

@@ -577,18 +580,5 @@ describe("dataset transform functions", () => {
expect(result[1][X_AXIS_DATA_KEY]).toBe(5);
expect(result[2][X_AXIS_DATA_KEY]).toBe(1000);
});

it("handles empty datasets without errors", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the context for this test being deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

It looks like this test here is the result of some rebase in the past because it was in the describe("getTransformedDataset", () => {}) group while testing applyVisualizationSettingsDataTransformations which is a newer name. And there is already a "should work on empty datasets" test of applyVisualizationSettingsDataTransformations

@alxnddr alxnddr merged commit a1e23a8 into combo-stacked-chart May 14, 2024
110 of 111 checks passed
@alxnddr alxnddr deleted the propagate-combo-stacking-to-areas-bars branch May 14, 2024 19:40
alxnddr added a commit that referenced this pull request May 14, 2024
* 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
alxnddr added a commit that referenced this pull request May 16, 2024
* combo stacked chart

* spec

* remove irrelevant spec

* combo stacking

* fix selecting y-axis on stacked charts

* snapshot

* linter, specs, types

* propagate combo stacking to area and bar charts (#42600)

* 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

* review

* show show data values setting on normalized charts that have a line series

* fixes
JesseSDevaney added a commit that referenced this pull request May 17, 2024
* move series data label formatters to the model

- TODO: move stacked data label formatters to the model

* combo stacked chart

* spec

* remove irrelevant spec

* combo stacking

* fix selecting y-axis on stacked charts

* snapshot

* linter, specs, types

* 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

* propagate combo stacking to area and bar charts (#42600)

* 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

* review

* remove unused formatting options parameter

* move stacked labels formatter to model

* fix stacked combo chart label display for non-stacked lines

* combo stacked chart

* spec

* remove irrelevant spec

* combo stacking

* fix selecting y-axis on stacked charts

* snapshot

* linter, specs, types

* propagate combo stacking to area and bar charts (#42600)

* 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

* review

* fix waterfall chart labels

---------

Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
Co-authored-by: Aleksandr Lesnenko <alxnddr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Team/DashViz Dashboard and Viz team visual Run Percy visual testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants