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

feature, Improves Grid chart labels and values responsivenes #1751

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

Passanelli
Copy link
Collaborator

What does this implement/fix?

Improves Grid chart labels and values to truncate when overflow containers through the truncateText utility function.
Update css to remove outline on Grid groups.
Increase Y_LABEL_OFFSET to put yLabel closer to the left edge of the chart container

Tophat 🎩 link

What do the changes look like?

Not sure why is not visible but there is a resizable container which you can play around:

Screen.Recording.2024-11-08.at.3.59.20.PM.mov

Storybook link

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 8, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.6 KB (0%) 1.3 s (0%) 1.4 s (+23.25% 🔺) 2.6 s
polaris-viz-cjs 222.36 KB (+0.16% 🔺) 4.5 s (+0.16% 🔺) 1.7 s (+5.47% 🔺) 6.2 s
polaris-viz-esm 180.28 KB (+0.08% 🔺) 3.7 s (+0.08% 🔺) 1.5 s (+20.92% 🔺) 5.1 s
polaris-viz-css 5.47 KB (+0.06% 🔺) 110 ms (+0.06% 🔺) 224 ms (+17.96% 🔺) 333 ms
polaris-viz-esnext 186.93 KB (+0.07% 🔺) 3.8 s (+0.07% 🔺) 1.6 s (+15.6% 🔺) 5.3 s

@Passanelli Passanelli force-pushed the passanelli/grid_responsiveness_improvements branch 2 times, most recently from 8c96470 to 10d6482 Compare November 8, 2024 19:12
@Passanelli Passanelli force-pushed the passanelli/grid_responsiveness_improvements branch 4 times, most recently from 49902fe to 12d93cd Compare November 11, 2024 15:43
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.

🎩 looks good

A couple nits and I wonder if you could use the existing truncation functions, but I think they are buried in the Labels component right now, so could be a future improvement

Also wonder if you should return nothing if the only string that would be returned would be the ellipsis like you can see in some of the boxes here

Screenshot 2024-11-11 at 1 56 21 PM

@@ -0,0 +1,23 @@
import {estimateStringWidth} from '@shopify/polaris-viz-core';

export function truncateText(
Copy link
Contributor

Choose a reason for hiding this comment

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

We do have some truncation utilities already, but they live in Label, so I guess you can't easily use them within this component right now 😢

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh i see the complexity to re use them 😢 .. but definitely something that we can tackle in a future improvement 💪🏼

@Passanelli Passanelli force-pushed the passanelli/grid_responsiveness_improvements branch from 12d93cd to c76729e Compare November 11, 2024 19:08
@Passanelli
Copy link
Collaborator Author

🎩 looks good

A couple nits and I wonder if you could use the existing truncation functions, but I think they are buried in the Labels component right now, so could be a future improvement

Also wonder if you should return nothing if the only string that would be returned would be the ellipsis like you can see in some of the boxes here

Screenshot 2024-11-11 at 1 56 21 PM

that's a good question, as far as i know, grid should not shrink too much since we are restricting the Card size to be 4x8 at minimum, so it should look like this at minimum size:

image

@carysmills
Copy link
Contributor

@Passanelli I think it could be small on mobile? Or if someone else uses this component on another surface

@Passanelli
Copy link
Collaborator Author

just validated on mobile and it looks good, but i think you are right about another surfaces, i will update it to return nothing in those scenarios, thanks again @carysmills 🙌🏼

@Passanelli Passanelli force-pushed the passanelli/grid_responsiveness_improvements branch from c76729e to 6437d04 Compare November 11, 2024 19:28
@carysmills carysmills merged commit d7ed495 into main Nov 11, 2024
5 of 6 checks passed
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