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

Add group sum labels for the horizontal stacked bar #1634

Merged

Conversation

rita-morozova
Copy link
Contributor

@rita-morozova rita-morozova commented Feb 29, 2024

What does this implement/fix?

While transitioning our reports to MAAPI, we needed to update the stacked horizontal bar chart to support group sum labels, a similar approach to what we have in the Simple Bar Chart.

Screenshot 2024-02-29 at 1 16 17 PM

Does this close any currently open issues?

Resolves https://github.com/Shopify/core-issues/issues/67692

What do the changes look like?

In our planned usage of this chart for UA, we will not display X-axis labels with vertical grid lines.

Screenshot 2024-02-29 at 1 16 57 PM

Storybook link

Stacked Horizontal Bar

  • Add positive and negative values to the data array and make sure nothing breaks

Stacked Horizontal Bar With No Group Labels

  • Make sure the group labels are not displayed here

Simple Bar Chart

  • Confirm it works as expected (the Simple Stacked Bar doesn't support the group sum labels)

Before merging

  • Check your changes on a variety of browsers and devices.

  • Update the Changelog's Unreleased section with your changes.

  • Update relevant documentation, tests, and Storybook.

  • Make sure you're exporting any new shared Components, Types and Utilities from the top level index file of the package

Copy link

github-actions bot commented Feb 29, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.36 KB (+0.02% 🔺) 1.3 s (+0.02% 🔺) 793 ms (+27.98% 🔺) 2.1 s
polaris-viz-cjs 215.92 KB (+0.12% 🔺) 4.4 s (+0.12% 🔺) 1.7 s (-12.78% 🔽) 6.1 s
polaris-viz-esm 175.91 KB (+0.09% 🔺) 3.6 s (+0.09% 🔺) 1.3 s (+4.41% 🔺) 4.8 s
polaris-viz-css 4.78 KB (0%) 96 ms (0%) 225 ms (-37.76% 🔽) 320 ms
polaris-viz-esnext 181.39 KB (+0.08% 🔺) 3.7 s (+0.08% 🔺) 1.3 s (+5.39% 🔺) 4.9 s

@@ -147,6 +185,17 @@ export function HorizontalStackedBars({
</Fragment>
);
})}
{!isSimple && (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just spit balling here, I wonder if we still want a way to show/hide labels even for non-simple charts.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@envex I do think we should be able to hide these labels. If the x-axis labels are displayed and we also have the group labels, it becomes overwhelming.

I will look into adding this functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@envex I added an option to the Theme that supports hiding group labels.

@rita-morozova rita-morozova requested a review from envex March 4, 2024 15:33
Copy link
Collaborator

@envex envex left a comment

Choose a reason for hiding this comment

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

Sorry, It looks like a UI test was broken: https://shopify.chromatic.com/test?appId=6062ad4a2d14cd0021539c1b&id=65e24099b59ea560c35266e0

Can you take a peek?

@rita-morozova rita-morozova force-pushed the rita-morozova/add-group-sum-labels-for-stacked-bar branch from af48f49 to 208b1c1 Compare March 4, 2024 17:50
@rita-morozova rita-morozova force-pushed the rita-morozova/add-group-sum-labels-for-stacked-bar branch from 208b1c1 to 61df7a0 Compare March 4, 2024 17:52
@rita-morozova rita-morozova requested a review from envex March 4, 2024 17:57
@rita-morozova rita-morozova merged commit 1894d36 into main Mar 4, 2024
6 checks passed
@rita-morozova rita-morozova deleted the rita-morozova/add-group-sum-labels-for-stacked-bar branch March 4, 2024 19:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants