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

LG-4764: De-emphasize other series lines when one is hovered #2650

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

tsck
Copy link
Collaborator

@tsck tsck commented Jan 22, 2025

✍️ Proposed changes

  • Adds config that causes other series lines to become opaque when a given series line is hovered over.
    • This differs from designs slightly. These were discussed and agreed upon with design.
      • The given line doesn't widen on hover. In order to always show the tooltip, we have the tooltip.trigger set to axis. Due to this, the emphasis event on a series occurs when a user hovers anywhere on the chart. So setting the line to a larger width on emphasis makes it so that all lines become wider when the cursor enters the chart.
      • When a point marker is hovered, it doesn't emphasize the line below it. Currently marker points are not associated with a series. I added the option to associate them with a series, but this didn't make it so that the associated series was deemed emphasized on hover of the mark point either. This just appears to be an echarts limitation. Ultimately the plan is to highlight the point and associated line in the custom tooltip when that is built out. I've reverted the ability to associate a point with a line for now until that functionality is necessary.
  • Drive-bys:
    • Improves the types of EventMarkerPoint and EventMarkerLine by distinguishing the position prop depending on which component it is.
    • Removes actions from storybook. This was a nice to have, but is causing errors since pnpm, I believe it might be a version mismatch with storybook. It didn't add a ton of value so I just removed it.

🎟 Jira ticket: LG-4764

✅ Checklist

For bug fixes, new features & breaking changes

  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • I have run pnpm changeset and documented my changes

For new components

  • I have added my new package to the global tsconfig
  • I have added my new package to the Table of Contents on the global README
  • I have verified the Live Example looks as intended on the design website.

🧪 How to test changes

  • Test in Storybook

Screenshot

Screenshot 2025-01-22 at 9 57 18 AM

@tsck tsck requested a review from a team as a code owner January 22, 2025 15:03
@tsck tsck requested review from shaneeza and removed request for a team January 22, 2025 15:03
Copy link

changeset-bot bot commented Jan 22, 2025

🦋 Changeset detected

Latest commit: ea5096c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lg-charts/core Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

disabled: true,
focus: 'series',
},
blur: {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to add a transition as well? If you move horizontally across the chart, it flickers a lot because of the opacity change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure there's a way to transition, but I actually don't know if a transition would actually help the blinking so much. I do think this is a good callout though and would be curious to get @sooachung's thoughts on this.

@sooachung Shaneeza raises a good point about how having this emphasis on the hovered line causes a great deal of blinking when the user hovers over the chart. Wanted to check in with you about this. (Example below)

Screen.Recording.2025-01-22.at.2.46.26.PM.mov

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sooa is checking with Visudha on this


export function EventMarkerLine({
position,
label,
message,
level = EventLevel.Warning,
}: EventMarkerLineProps) {
}: Omit<EventMarkerLineProps, 'type'>) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does EventMarkerLineProps include type if it needs to be omitted? Is it only needed for BaseEventMarker?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This might be wonky but it exists so that it can be reused by BaseEventMarker, allowing BaseEventMarker to accept EventMarkerLineProps | EventMarkerPointProps, which allows a check such as if (type === 'point') position[1] in BaseEventMarker since TS will know if position is an array or not based on the type.

Essentially what it amounts to is:

export interface BaseEventMarkerProps {
  label?: string;
  message: string;
  level: EventLevel;
  position: [string | number, string | number] | string | number;
  type: 'line' | 'point';
}

export interface BaseEventMarkerLineProps
  extends Omit<BaseEventMarkerProps, 'name' | 'theme'> {
  type: 'line';
  position: string | number;
}

export interface BaseEventMarkerPointProps
  extends Omit<BaseEventMarkerProps, 'name' | 'theme'> {
  type: 'point';
  position: [string | number, string | number];
}

export interface EventMarkerLineProps extends Omit<BaseEventMarkerLineProps, 'type'> {}

export interface EventMarkerPointProps extends Omit<BaseEventMarkerPointProps, 'type'> {}

But isntead I'm inlining the bottom two interfaces.

I'm definitely open to suggestions to making this cleaner!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, I think being more explicit like the example I gave makes more sense since we do ultimately export EventMarkerLineProps and EventMarkerPointProps. So without this, it's actually currently exporting the incorrect type. Will push a fix for this

'@lg-charts/core': patch
---

Fixes series emphasis on line hover by de-emphasizing other lines
Copy link
Collaborator

@shaneeza shaneeza Jan 22, 2025

Choose a reason for hiding this comment

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

Can you also add the type changes?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't add that because it doesn't actually change what's exposed, it just cleans up the type internally for the base component so that position can be determined by type. But the exposed types should be the same. Is this something that we'd normally note here?

Copy link
Collaborator

@shaneeza shaneeza Jan 23, 2025

Choose a reason for hiding this comment

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

It might be worth noting things that have changed internally. In case something breaks, it would be easier to track it. However, I don't feel strongly about this, so if you don't see the need to add it, that's fine with me.

@tsck tsck requested a review from shaneeza January 22, 2025 20:09
label?: string;
message: string;
level: EventLevel;
position: [string | number, string | number] | string | number;
type: 'line' | 'point';
}

export interface BaseEventMarkerLineProps
extends Omit<BaseEventMarkerProps, 'name' | 'theme'> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't look like name and theme are included in BaseEventMarkerProps

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