-
Notifications
You must be signed in to change notification settings - Fork 25
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
Conversation
size-limit report 📦
|
8c96470
to
10d6482
Compare
packages/polaris-viz/src/components/Grid/components/GroupInfo.tsx
Outdated
Show resolved
Hide resolved
packages/polaris-viz/src/components/Grid/components/GroupInfo.tsx
Outdated
Show resolved
Hide resolved
packages/polaris-viz/src/components/Grid/utilities/truncate-text.ts
Outdated
Show resolved
Hide resolved
49902fe
to
12d93cd
Compare
There was a problem hiding this 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
packages/polaris-viz/src/components/Grid/components/GroupInfo.tsx
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,23 @@ | |||
import {estimateStringWidth} from '@shopify/polaris-viz-core'; | |||
|
|||
export function truncateText( |
There was a problem hiding this comment.
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 😢
There was a problem hiding this comment.
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 💪🏼
12d93cd
to
c76729e
Compare
@Passanelli I think it could be small on mobile? Or if someone else uses this component on another surface |
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 🙌🏼 |
c76729e
to
6437d04
Compare
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