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

Rename dimensions to containerBounds #1757

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

envex
Copy link
Collaborator

@envex envex commented Nov 19, 2024

What does this implement/fix?

Note

I'd like to merge this before #1755 & #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.

image

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

Copy link

github-actions bot commented Nov 19, 2024

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%) 1.1 s (+22.69% 🔺) 2.4 s
polaris-viz-cjs 223.04 KB (+0.1% 🔺) 4.5 s (+0.1% 🔺) 2 s (+29.81% 🔺) 6.4 s
polaris-viz-esm 180.94 KB (+0.16% 🔺) 3.7 s (+0.16% 🔺) 1.4 s (+4.4% 🔺) 5 s
polaris-viz-css 5.56 KB (0%) 112 ms (0%) 338 ms (-40.04% 🔽) 449 ms
polaris-viz-esnext 187.71 KB (+0.17% 🔺) 3.8 s (+0.17% 🔺) 1.4 s (+8.97% 🔺) 5.2 s

@envex envex force-pushed the envex/container-bounds branch 2 times, most recently from b3a3234 to abe7fab Compare November 19, 2024 13:14
@envex envex marked this pull request as ready for review November 19, 2024 13:21
Copy link
Contributor

@carysmills carysmills left a comment

Choose a reason for hiding this comment

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

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.

Also just going back to the original problem:

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.

Would it possibly make sense to just ensure that one prop always has all the dimensions we are looking for, rather than having two props that are similar?

const {legend, setLegendDimensions, height, width} = useLegend({
data: [
{
shape: 'Line',
series: data,
},
],
dimensions,
dimensions: containerDimensions,
Copy link
Contributor

Choose a reason for hiding this comment

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

This line used to be dimensions and so did line 261, so I am a bit confused why they are replaced with different variables, not the same one. Is this intentional? Were we using the wrong variable before?

Copy link
Collaborator Author

@envex envex Nov 19, 2024

Choose a reason for hiding this comment

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

This is intentional. This was using the original dimensions prop that was actually bounds.

Now containerDimensions only passes height and width.

@envex
Copy link
Collaborator Author

envex commented Nov 19, 2024

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.

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