fix(chart): Time Series set showMaxLabel as null for time xAxis #20627

Antonio-RiveroMartnez · 2022-07-06T17:46:05Z

SUMMARY

Apache Echarts has showMaxLabel option set to false by default for time axis (https://github.com/apache/echarts/blob/master/src/coord/axisDefault.ts#L178), so we need to override it for our time series charts in order to not be fixed with hidden and instead let the library determine whether to show the label of the max tick
Add AxisType enum so we stop comparing agains raw strings when checking xAxis type

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

BEFORE:

AFTER:

TESTING INSTRUCTIONS

Go to a time series line chart with monthly data from the example datasets
Chose month time grain
Expected results: last month should show on X axis

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

codecov · 2022-07-06T18:22:28Z

Codecov Report

Merging #20627 (3169da9) into master (8d4994a) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #20627      +/-   ##
==========================================
- Coverage   66.85%   66.85%   -0.01%     
==========================================
  Files        1753     1753              
  Lines       65825    65827       +2     
  Branches     7006     7007       +1     
==========================================
  Hits        44010    44010              
- Misses      20030    20031       +1     
- Partials     1785     1786       +1

Flag	Coverage Δ
javascript	`51.95% <0.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...gin-chart-echarts/src/Timeseries/transformProps.ts	`54.65% <0.00%> (-1.31%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d4994a...3169da9. Read the comment docs.

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

rusackas · 2022-07-07T19:02:42Z

/testenv up

github-actions · 2022-07-07T19:03:31Z

@rusackas Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-07-07T19:03:32Z

@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.

rusackas · 2022-07-07T20:28:39Z

/testenv up

github-actions · 2022-07-07T20:29:18Z

@rusackas Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-07-07T20:29:20Z

@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.

Antonio-RiveroMartnez · 2022-07-11T20:54:21Z

🏷️ preset-patch

stephenLYZ · 2022-07-12T02:49:05Z

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

@@ -326,13 +327,23 @@ export default function transformProps(
      rotate: xAxisLabelRotation,


Suggested change

rotate: xAxisLabelRotation,

rotate: xAxisLabelRotation,

Suggested change

rotate: xAxisLabelRotation,

rotate: xAxisLabelRotation,

showLabel: xAxisType === AxisType.time ? null : false,

looks like we can use one line to meet the requirement 🤔

If you do that you will be overriding default values for other axis types to false. Which might not be the desire output and might impact how other axis behave?. I would rather let the defaults for other types be set in Echarts and respect their criteria and being explicit to overriding only when/where we need to.

Yeah. This point makes sense. Another I'm curious why not set showMaxLabel option here?

How so? I mean, setting showMaxLabel here (in this initial declaration) is precisely what I avoid doing so we don't override the defaults for other types (this initial value gets used for all xAxis types). 🤔

Actually, you made me realize I call it showLabel and not showMaxLabel with my second commit. Let me fix that ASAP

Another suggestion is we can try to set FEATURE_GENERIC_CHART_AXES=true to see if it also works.

I may be mistaken, however, based on this from the README:

A new feature flag GENERIC_CHART_AXES has been added that makes it possible to use a non-temporal x-axis on the ECharts Timeseries chart ([#17917](https://github.com/apache/superset/pull/17917)). When enabled, a new control "X Axis" is added to the control panel of ECharts line, area, bar, step and scatter charts, which makes it possible to use categorical or numerical x-axes on those charts.

If that's true those are exactly the axis types we don't want to include with these changes. Nonetheless, let me try setting it as true and see if it actually fix the issue by itself.

@stephenLYZ enabling that flag we get the last month to show but the chart looks off to me and the format is not correct:

Changes from this PR:

No changes + Flag enabled:

That second one does look odd... is there a way to solve for that? If there's no 0 month, it seems like that shouldn't be on the X axis.

Sorry for the misunderstanding. I was showing the result for the approach mentioned above about enabling the flag (with no code changes).

So the first image shows the chart with the code changes in this PR

stephenLYZ · 2022-07-12T02:50:41Z

/testenv up FEATURE_GENERIC_CHART_AXES=true

github-actions · 2022-07-12T02:51:46Z

@stephenLYZ Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-07-12T02:51:47Z

@stephenLYZ Ephemeral environment creation failed. Please check the Actions logs for details.

-Apache echarts has this option as false by default for time axis, so we need to override it for our charts so it's uto determined and not fixed to hidden. - Add AxisType enum so we stop comparing agains raw strings when checking xAxis type

- set the showMaxLabel option directly without using merge

- Rename the property to showMaxLabel as it was originally

rusackas · 2022-08-02T20:29:48Z

/testenv up FEATURE_GENERIC_CHART_AXES=true

github-actions · 2022-08-02T20:29:58Z

@rusackas Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-08-02T20:29:59Z

@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.

github-actions · 2022-08-02T20:30:31Z

@rusackas Container image not yet published for this PR. Please try again when build is complete.

github-actions · 2022-08-02T20:30:32Z

@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.

rusackas

Ephemeral environments are obviously flaky here, but based on the code/screenshots/discussion, I think we're OK here.

villebro · 2023-02-20T10:12:42Z

@Antonio-RiveroMartnez I've noticed that this PR has caused time series charts to show weird max labels when the chart is wide enough:

Removing showMaxLabel: null resolves it:

IIRC, setting showMaxLabel: null makes it behave the same as if it was set to showMaxLabel: true. Therefore, I believe we should revert this change and consider alternative solutions for solving the problem this initially tried to fix.

Ping also reviewers @rusackas and @diegomedina248

Antonio-RiveroMartnez · 2023-02-21T02:56:01Z

Hey @villebro , thanks for the catch 🙌 , I'm more than happy reverting the changes if they are causing issues.

showMaxLabel: null means ECharts automatically determine whether or not the label should display, while setting it to true would force the label to show all the time. For time series, it's set to false by default, that's why we tried to bypass such wrong behavior by setting it to null and rely on such auto computation.

Now, I might be wrong, but finding a new approach might get us to address the issue directly on ECharts, since it is the one in charge for how to render such labels.

rusackas · 2023-04-06T21:30:03Z

I just happened to stumble upon this thread. It doesn't look like this was reverted, in case this is causing any further issues/complaints (cc @yousoph for good measure)

yousoph · 2023-04-07T21:45:22Z

Oh! Thanks, yeah I don't think we reverted it or fixed the extra timestamp label. We'll take a look

mpostelnicu · 2023-08-03T19:16:31Z

if possible, please revert this in 2.1.1 because of the extra timestamp label displayed at the end

rusackas · 2023-08-12T03:36:44Z

The issue is labelled for 2.1 already, but I'll CC @Antonio-RiveroMartnez and @eschutho for good measure. I'm not sure if it's easier at this point to revert or to fix forward, so I'll leave the choice to them.

…is (apache#20627)" This reverts commit 9362e27.

…e xAxis (#20627) (#24995)

…e xAxis (apache#20627) (apache#24995) (cherry picked from commit 2b63577)

…e xAxis (#20627) (#24995) (cherry picked from commit 2b63577)

…e xAxis (#20627) (#24995)

…e xAxis (apache#20627) (apache#24995) (cherry picked from commit 2b63577)

…e xAxis (apache#20627) (apache#24995)

pull-request-size bot added the size/S label Jul 6, 2022

Antonio-RiveroMartnez force-pushed the FIX-49973 branch from 49829e3 to f45157e Compare July 7, 2022 10:57

diegomedina248 reviewed Jul 7, 2022

View reviewed changes

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts Outdated Show resolved Hide resolved

diegomedina248 approved these changes Jul 7, 2022

View reviewed changes

Antonio-RiveroMartnez force-pushed the FIX-49973 branch 2 times, most recently from 4d489ea to 9fe79a1 Compare July 7, 2022 18:15

Antonio-RiveroMartnez force-pushed the FIX-49973 branch from 9fe79a1 to 3bd419f Compare July 8, 2022 12:20

rusackas added the Preset-Patch label Jul 11, 2022

stephenLYZ reviewed Jul 12, 2022

View reviewed changes

Antonio-RiveroMartnez added 3 commits July 12, 2022 09:54

Time Series Chart:

3ae970c

-Apache echarts has this option as false by default for time axis, so we need to override it for our charts so it's uto determined and not fixed to hidden. - Add AxisType enum so we stop comparing agains raw strings when checking xAxis type

Time Series Chart:

038e2ff

- set the showMaxLabel option directly without using merge

Time Series Chart:

3169da9

- Rename the property to showMaxLabel as it was originally

Antonio-RiveroMartnez force-pushed the FIX-49973 branch from 3bd419f to 3169da9 Compare July 12, 2022 12:56

rusackas approved these changes Aug 2, 2022

View reviewed changes

rusackas merged commit 9362e27 into apache:master Aug 2, 2022

Antonio-RiveroMartnez deleted the FIX-49973 branch August 2, 2022 21:15

yousoph mentioned this pull request Jun 23, 2023

Timestamp appears at end of x-axis for ECharts with long filtered interval #23197

Closed

eschutho added a commit to preset-io/superset that referenced this pull request Aug 15, 2023

Revert "fix(chart): Time Series set showMaxLabel as null for time xAx…

653a403

…is (apache#20627)" This reverts commit 9362e27.

eschutho mentioned this pull request Aug 15, 2023

fix: Revert "fix(chart): Time Series set showMaxLabel as null for time xAxis (#20627) #24995

Merged

9 tasks

john-bodley pushed a commit that referenced this pull request Aug 16, 2023

fix: Revert "fix(chart): Time Series set showMaxLabel as null for tim…

2b63577

…e xAxis (#20627) (#24995)

jinghua-qa pushed a commit to preset-io/superset that referenced this pull request Aug 16, 2023

fix: Revert "fix(chart): Time Series set showMaxLabel as null for tim…

0ad64fb

…e xAxis (apache#20627) (apache#24995) (cherry picked from commit 2b63577)

michael-s-molina pushed a commit that referenced this pull request Aug 17, 2023

fix: Revert "fix(chart): Time Series set showMaxLabel as null for tim…

e20c296

…e xAxis (#20627) (#24995) (cherry picked from commit 2b63577)

eschutho added a commit that referenced this pull request Aug 17, 2023

fix: Revert "fix(chart): Time Series set showMaxLabel as null for tim…

8a92c92

…e xAxis (#20627) (#24995)

eschutho added a commit that referenced this pull request Aug 18, 2023

fix: Revert "fix(chart): Time Series set showMaxLabel as null for tim…

2f19b12

…e xAxis (#20627) (#24995)

sfirke mentioned this pull request Aug 28, 2023

Last date/time label missing on chart X axis #25099

Open

mpostelnicu pushed a commit to devgateway/ocmoldova-superset that referenced this pull request Aug 31, 2023

fix: Revert "fix(chart): Time Series set showMaxLabel as null for tim…

3593880

…e xAxis (apache#20627) (apache#24995) (cherry picked from commit 2b63577)

mistercrunch added the 🚢 2.1.3 label Feb 18, 2024

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 and removed 🚢 2.1.3 labels Mar 13, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix: Revert "fix(chart): Time Series set showMaxLabel as null for tim…

81f1842

…e xAxis (apache#20627) (apache#24995)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(chart): Time Series set showMaxLabel as null for time xAxis #20627

fix(chart): Time Series set showMaxLabel as null for time xAxis #20627

Antonio-RiveroMartnez commented Jul 6, 2022

codecov bot commented Jul 6, 2022 •

edited

Loading

rusackas commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

rusackas commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

Antonio-RiveroMartnez commented Jul 11, 2022

stephenLYZ Jul 12, 2022

Antonio-RiveroMartnez Jul 12, 2022 •

edited

Loading

stephenLYZ Jul 12, 2022

Antonio-RiveroMartnez Jul 12, 2022

Antonio-RiveroMartnez Jul 12, 2022 •

edited

Loading

stephenLYZ Jul 12, 2022

Antonio-RiveroMartnez Jul 12, 2022

Antonio-RiveroMartnez Jul 12, 2022 •

edited

Loading

rusackas Jul 13, 2022

Antonio-RiveroMartnez Jul 13, 2022 •

edited

Loading

stephenLYZ commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

rusackas commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

rusackas left a comment

villebro commented Feb 20, 2023

Antonio-RiveroMartnez commented Feb 21, 2023

rusackas commented Apr 6, 2023

yousoph commented Apr 7, 2023

mpostelnicu commented Aug 3, 2023

rusackas commented Aug 12, 2023

		@@ -326,13 +327,23 @@ export default function transformProps(
		rotate: xAxisLabelRotation,

	rotate: xAxisLabelRotation,
	rotate: xAxisLabelRotation,
	showLabel: xAxisType === AxisType.time ? null : false,

fix(chart): Time Series set showMaxLabel as null for time xAxis #20627

fix(chart): Time Series set showMaxLabel as null for time xAxis #20627

Conversation

Antonio-RiveroMartnez commented Jul 6, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 6, 2022 • edited Loading

Codecov Report

rusackas commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

rusackas commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

github-actions bot commented Jul 7, 2022

Antonio-RiveroMartnez commented Jul 11, 2022

stephenLYZ Jul 12, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

stephenLYZ Jul 12, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jul 12, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

stephenLYZ Jul 12, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jul 12, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jul 12, 2022 • edited Loading

Choose a reason for hiding this comment

rusackas Jul 13, 2022

Choose a reason for hiding this comment

Antonio-RiveroMartnez Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

stephenLYZ commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

github-actions bot commented Jul 12, 2022

rusackas commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

github-actions bot commented Aug 2, 2022

rusackas left a comment

Choose a reason for hiding this comment

villebro commented Feb 20, 2023

Antonio-RiveroMartnez commented Feb 21, 2023

rusackas commented Apr 6, 2023

yousoph commented Apr 7, 2023

mpostelnicu commented Aug 3, 2023

rusackas commented Aug 12, 2023

codecov bot commented Jul 6, 2022 •

edited

Loading

Antonio-RiveroMartnez Jul 12, 2022 •

edited

Loading

Antonio-RiveroMartnez Jul 12, 2022 •

edited

Loading

Antonio-RiveroMartnez Jul 12, 2022 •

edited

Loading

Antonio-RiveroMartnez Jul 13, 2022 •

edited

Loading