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

fix: adjust line type as well as weight for time series #30949

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

Conversation

eschutho
Copy link
Member

@eschutho eschutho commented Nov 16, 2024

SUMMARY

When there are more than 5 time series in a line chart, the line weights tend to get very thick. I'm stopping the line weight at five and then starting to adjust the dashed pattern instead.

I also removed the line boldness on hover because on dashed lines the animation is a bit distracting.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before:
image
image

After:

Screenshot 2024-11-15 at 5 32 17 PM Screenshot 2024-11-20 at 9 57 05 AM

Hover animation:
Before:

Screen.Recording.2024-11-15.at.5.39.03.PM.mov

After:

Screen.Recording.2024-11-15.at.5.41.34.PM.mov

TESTING INSTRUCTIONS

Create a line chart with a time comparison and add more than 5 time comparisons.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

@dosubot dosubot bot added the viz:charts:timeseries Related to Timeseries label Nov 16, 2024
Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM!

// https://apache.github.io/echarts-handbook/en/basics/release-note/5-3-0/#removing-the-default-bolding-emphasis-effect-in-the-line-chart
// TODO: should consider only adding emphasis to currently hovered series
lineStyle: {
width: 'bolder',
Copy link
Member Author

Choose a reason for hiding this comment

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

@villebro is it ok to remove this?

Copy link
Member

Choose a reason for hiding this comment

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

I also removed the line boldness on hover because on dashed lines the animation is a bit distracting.

@eschutho I believe this was added to keep the hover behavior consistent with other chart types.

@eschutho
Copy link
Member Author

cc @michael-s-molina

@@ -288,11 +288,20 @@ export default function transformProps(
entry,
ensureIsArray(chartProps.rawFormData?.time_compare),
)!;
if (!offsetLineWidths[offset]) {
offsetLineWidths[offset] = Object.keys(offsetLineWidths).length + 1;
if (!offsetLineWidths.includes(offset)) {
Copy link
Member

@michael-s-molina michael-s-molina Nov 19, 2024

Choose a reason for hiding this comment

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

The previous map approach is more efficient than looping through the elements with include.

@eschutho eschutho added the hold! On hold label Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants