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

feat: Make time shifted series colors match the original series #24048

Merged
merged 3 commits into from
May 18, 2023

Conversation

michael-s-molina
Copy link
Member

@michael-s-molina michael-s-molina commented May 12, 2023

SUMMARY

This PR upgrades Timeseries ECharts to display time shifted series as dashed lines and using the same color as the original series.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2023-05-12.at.15.30.22.mov

TESTING INSTRUCTIONS

Check the video for instructions.

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

isString(series.name)
? !!timeCompare.find(
timeOffset =>
// offset is represented as <offset>, group by list
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 I tried changing the way metrics with and without dimensions are displayed but ended up in a rabbit hole. I decided to restrain the scope of the PR. If we decide to change that later, it will be easy to simplify this code.

Copy link
Member

@villebro villebro May 12, 2023

Choose a reason for hiding this comment

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

Sounds good, I can take a stab at it, too 👍

@@ -227,30 +230,43 @@ export default function transformProps(
contributionMode || isAreaExpand ? ',.0%' : yAxisFormat,
);

const array = ensureIsArray(chartProps.rawFormData?.time_compare);
const inverted = invert(verboseMap);
Copy link
Member Author

Choose a reason for hiding this comment

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

The data series are transformed to use the verbose name but I need the original name to compare against the result.

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #24048 (40ca111) into master (c8beaab) will increase coverage by 0.08%.
The diff coverage is 69.67%.

❗ Current head 40ca111 differs from pull request most recent head eaa3ae7. Consider uploading reports for the commit eaa3ae7 to get more accurate results

@@            Coverage Diff             @@
##           master   #24048      +/-   ##
==========================================
+ Coverage   68.13%   68.22%   +0.08%     
==========================================
  Files        1941     1943       +2     
  Lines       75304    75225      -79     
  Branches     8166     8151      -15     
==========================================
+ Hits        51312    51325      +13     
+ Misses      21903    21814      -89     
+ Partials     2089     2086       -3     
Flag Coverage Δ
javascript 54.52% <65.88%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...hart-echarts/src/MixedTimeseries/transformProps.ts 0.00% <0.00%> (ø)
...lugin-chart-echarts/src/Timeseries/transformers.ts 56.95% <ø> (-0.29%) ⬇️
superset-frontend/src/SqlLab/actions/sqlLab.js 66.41% <ø> (-2.35%) ⬇️
...frontend/src/SqlLab/components/ResultSet/index.tsx 63.05% <ø> (+0.39%) ⬆️
...et-frontend/src/components/TableSelector/index.tsx 81.08% <0.00%> (ø)
superset/charts/api.py 85.85% <ø> (ø)
superset/connectors/sqla/models.py 87.24% <ø> (+0.13%) ⬆️
superset/dashboards/api.py 92.58% <ø> (ø)
superset/databases/api.py 91.58% <ø> (ø)
superset/datasets/api.py 88.00% <ø> (ø)
... and 33 more

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM and tested to work as expected. I can try to do the next round of improvements.

rawSeries.forEach(entry => {
const lineStyle = isDerivedSeries(entry, chartProps.rawFormData)
? { type: 'dashed' as ZRLineType }
: {};
const transformedSeries = transformSeries(entry, colorScale, {
Copy link
Member

Choose a reason for hiding this comment

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

A thought for future improvement - we may want to use different patterns to disambiguate the different derived series from one another. dasharray could be useful to create half a dozen or so different patterns:

Copy link
Member

Choose a reason for hiding this comment

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

@villebro we did a quick hackathon on this, and the echart option works great. We haven't put up a PR yet because there's more that needs to be done around toggling the visualization on/off per user.

Copy link
Member

Choose a reason for hiding this comment

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

@eschutho nice, this would be a really nice improvement! ❤️

@michael-s-molina michael-s-molina merged commit df4d16a into apache:master May 18, 2023
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 8, 2024
@azybareva
Copy link

azybareva commented Mar 19, 2024

For Mixed Series Diagrams the time shifted line looks absolutely like original
А dotted line would be preferable
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants