fix: adjust line type as well as weight for time series #30949
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eschutho
commented
Nov 18, 2024
| // 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', |
There was a problem hiding this comment.
@villebro is it ok to remove this?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
How do you feel about us removing the bolder weight? It looks like the echarts community didn't like the look of it on the line chart, so they removed it. It just seems more distracting than anything on the line chart imo.
| @@ -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)) { | |||
There was a problem hiding this comment.
The previous map approach is more efficient than looping through the elements with include.
eschutho
force-pushed
the
elizabeth/adjust-line-weight
branch
from
879919b to
5a7feff
Compare
eschutho
force-pushed
the
elizabeth/adjust-line-weight
branch
from
5a7feff to
fcc9656
Compare
kgabryje
approved these changes
Jan 20, 2025
eschutho
force-pushed
the
elizabeth/adjust-line-weight
branch
from
fcc9656 to
82603e6
Compare
hainenber
pushed a commit
to hainenber/superset
that referenced
this pull request
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
The line thicknesses were with time adjustments were making the visualization difficult to read. Instead, I kept the existing thickness to match the legacy line chart and adjusted 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:
After:
Hover animation:
Before:
Screen.Recording.2024-11-15.at.5.39.03.PM.mov
After:
Screen.Recording.2025-01-17.at.5.50.33.PM.mov
TESTING INSTRUCTIONS
Create a line chart with a time comparison and add more than 5 time comparisons.
ADDITIONAL INFORMATION