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 dashed support for legends #1773

Merged
merged 4 commits into from
Dec 5, 2024
Merged

Conversation

mollerjorge
Copy link
Collaborator

@mollerjorge mollerjorge commented Dec 4, 2024

What does this implement/fix?

  1. We need to add support for showing a dashed line in the legend. The use case is for benchmarks where the Median line is dashed, so the legend should follow the same pattern.
  2. Fixed Annotations textAnchor property, it shouldn't be left middle right but start middle end
  3. Fix text alignment on annotations: Should be centered

Before:
Screenshot 2024-12-04 at 10 44 27 AM

After:
Screenshot 2024-12-04 at 10 44 07 AM

🎩 Tophatting:

  1. Annotations text alignment: https://x5mqehaaczjinobf.tunnel.shopifycloud.tech/?path=/story/polaris-viz-charts-linechart--annotations
  2. Dashed legend in line chart relational: https://x5mqehaaczjinobf.tunnel.shopifycloud.tech/?path=/story/polaris-viz-charts-linechartrelational--default

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 Dec 4, 2024

size-limit report 📦

Path Size Loading time (3g) Running time (snapdragon) Total time
polaris-viz-core-cjs 61.73 KB (0%) 1.3 s (0%) 688 ms (-19.58% 🔽) 2 s
polaris-viz-cjs 223.94 KB (+0.03% 🔺) 4.5 s (+0.03% 🔺) 2.2 s (+27.86% 🔺) 6.7 s
polaris-viz-esm 181.37 KB (+0.03% 🔺) 3.7 s (+0.03% 🔺) 1.2 s (+0.08% 🔺) 4.8 s
polaris-viz-css 5.69 KB (0%) 114 ms (0%) 389 ms (-31.06% 🔽) 503 ms
polaris-viz-esnext 188.28 KB (+0.04% 🔺) 3.8 s (+0.04% 🔺) 1.3 s (-22.89% 🔽) 5.1 s

@mollerjorge mollerjorge force-pushed the jorge/add-dashed-support-in-legend branch from a0f8792 to e3e296c Compare December 4, 2024 13:48
@mollerjorge mollerjorge force-pushed the jorge/add-dashed-support-in-legend branch from e3e296c to f6ee802 Compare December 4, 2024 13:54
@mollerjorge mollerjorge requested a review from envex December 4, 2024 13:54
@mollerjorge mollerjorge marked this pull request as ready for review December 4, 2024 13:55
@@ -105,6 +105,10 @@ export const DEFAULT_DATA: DataSeries[] = [
{value: 21, key: '2020-03-14T12:00:00'},
],
color: LIGHT_THEME.seriesColors.limited[5],
metadata: {
Copy link
Collaborator Author

@mollerjorge mollerjorge Dec 4, 2024

Choose a reason for hiding this comment

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

using metadata for this looks like a good fit after checking others like:

metadata: {
   relatedIndex: 2,
   areaColor: 'rgba(218, 182, 242, 0.2)',
   legendLabel: '75th - 25th percentile',
},

@mollerjorge mollerjorge self-assigned this Dec 4, 2024
}: Props) {
const selectedTheme = useTheme();

export function SeriesIcon({color, lineStyle, shape = 'Bar'}: Props) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thought of refactoring this a bit, it didn't feel like SeriesIcon should concern about wether if its a comparison or not, I think we should pass in the color and lineStyle instead and calculate those from the parents

@mollerjorge mollerjorge force-pushed the jorge/add-dashed-support-in-legend branch from d76fb81 to 1ab29e4 Compare December 4, 2024 17:14
@mollerjorge mollerjorge requested a review from envex December 4, 2024 17:16
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.

Code looks good to me except the changelog
Did not 🎩 as @envex has more context on this chart and would be good for him to have a quick look

@mollerjorge mollerjorge force-pushed the jorge/add-dashed-support-in-legend branch from 2547296 to 5d28f30 Compare December 4, 2024 22:25
@mollerjorge
Copy link
Collaborator Author

Code looks good to me except the changelog Did not 🎩 as @envex has more context on this chart and would be good for him to have a quick look

Thanks @carysmills ! I added a link to tophat so its easier now, and added a fix for annotations as well.

@@ -16,7 +16,7 @@ interface SingleTextLineProps {
y: number;
ariaHidden?: boolean;
dominantBaseline?: 'middle' | 'hanging';
textAnchor?: 'left' | 'center' | 'right';
textAnchor?: 'start' | 'middle' | 'end';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mollerjorge mollerjorge force-pushed the jorge/add-dashed-support-in-legend branch from 5d28f30 to fc412a3 Compare December 4, 2024 22:39
@mollerjorge mollerjorge merged commit 8d1c79f into main Dec 5, 2024
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.

3 participants