Rename dimensions to containerBounds

[envex]

What does this implement/fix?

[!NOTE]
I'd like to merge this before https://github.com/Shopify/polaris-viz/pull/1755 & https://github.com/Shopify/polaris-viz/pull/1756

We're using dimensions and bounds all over the repo, but along the way sometimes a bounds prop will only contain dimensions and vice versa.

This PR renames the dimension prop to containerBounds so we can easily distinguish between what's a container value and whats a chart value.

In this example, the red bounds are considered containerBounds. This gives us the height and width as well as the x/y position of the container on the page.

We also have a concept of chartBounds, which is the height/width/x/y of the red outline. This is the bounds of the chart itself relative to the container.

What do the changes look like?

There should be no visual changes.

Storybook link

https://6062ad4a2d14cd0021539c1b-kjvrjvketz.chromatic.com/

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

[github-actions[bot]]

size-limit report 📦

Path	Size	Loading time (3g)	Running time (snapdragon)	Total time
polaris-viz-core-cjs	61.63 KB (0%)	1.3 s (0%)	853 ms (-10.44% 🔽)	2.1 s
polaris-viz-cjs	223.15 KB (+0.15% 🔺)	4.5 s (+0.15% 🔺)	2.3 s (+27.72% 🔺)	6.7 s
polaris-viz-esm	180.93 KB (+0.16% 🔺)	3.7 s (+0.16% 🔺)	2.2 s (+35.7% 🔺)	5.8 s
polaris-viz-css	5.55 KB (-0.11% 🔽)	112 ms (-0.11% 🔽)	266 ms (-13.63% 🔽)	377 ms
polaris-viz-esnext	187.7 KB (+0.16% 🔺)	3.8 s (+0.16% 🔺)	1.4 s (-17.41% 🔽)	5.2 s

[envex]

Are the height and width of containerBounds and chartBounds sometimes different? If they are the same, I am wondering if we can just have height and width associated with one of them, to avoid confusion.

The containerBounds and chartBounds will almost always be different.

containerBounds: x/y are relative to the browser. height/width are the entire size of the container. chartBounds: x/y are relative to the container and take into account the theme padding. height/width are the drawable area of the chart, so mainly container width - yAxis labels.

[envex]

@susiekims I renamed the props and ChartDimensions component based on your feedback.

[envex]

I hate myself for doing this, but I wonder if we even need containerDimensions to be it's own thing.

Most places we should be able to just pass down containerBounds even though we only need the height/width.

Thoughts?

[susiekims]

I hate myself for doing this, but I wonder if we even need containerDimensions to be it's own thing.

Most places we should be able to just pass down containerBounds even though we only need the height/width.

Thoughts?

@envex I was thinking about that actually! We don't really need chartDimensions and chartBounds, or containerDimensions and containerBounds, since bounds has dimensions. Maybe we could even store it chart context so we don't have to prop drill it down?

That being said, I'm happy to let this PR go in first and come back to it since it's still a step in a right direction and will make that refactor way easier.

[envex]

@susiekims Yeah, let's do all that in a follow up PR so we don't have this one hanging around blocking the other PRs.

[tiagoskaneta]

Hi, how should the dimensions be set now? Before I was using:

<SparkBarChart dimensions={{...}} />

But that is no longer available nor is the containerBounds replacement mentioned int this PR present in the SparkBarChartProps as far as I can tell.

Is there something I'm missing? Could not find any mention to this breaking change in the change logs. Not ideal as well that this was release as a patch version.

Thank you in advance.

[carysmills]

Hey @tiagoskaneta, you're right, these changes should have been listed as breaking changes. Sorry we missed that.

An alternative to setting the component size is adding a container element around the component with your width and height set, so that the component gets its size from the container. This is the suggested way of setting the chart size and is described here. Does that help? Let us know whether that works for your use case.

[tiagoskaneta]

An alternative to setting the component size is adding a container element around the component with your width and height set, so that the component gets its size from the container

thx @carysmills, yes, this should indeed be enough for my use case. thanks for following it up.