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.

Sign up for GitHub

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(chart): Time Series set showMaxLabel as null for time xAxis #20627

Merged
merged 3 commits into from
Aug 2, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ae970c
Time Series Chart:
Antonio-RiveroMartnez Jul 6, 2022
038e2ff
Time Series Chart:
Antonio-RiveroMartnez Jul 7, 2022
3169da9
Time Series Chart:
Antonio-RiveroMartnez Jul 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
EchartsTimeseriesSeriesType,
TimeseriesChartTransformedProps,
OrientationType,
AxisType,
} from './types';
import { DEFAULT_FORM_DATA } from './constants';
import { ForecastSeriesEnum, ForecastValue } from '../types';
Expand Down Expand Up @@ -329,13 +330,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Copy link
Member

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member Author

@Antonio-RiveroMartnez Antonio-RiveroMartnez Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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). 🤔

Copy link
Member Author

@Antonio-RiveroMartnez Antonio-RiveroMartnez Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Antonio-RiveroMartnez Antonio-RiveroMartnez Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@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:
PR

No changes + Flag enabled:
Flag

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@Antonio-RiveroMartnez Antonio-RiveroMartnez Jul 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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

},
minInterval:
xAxisType === 'time' && timeGrainSqla
xAxisType === AxisType.time && timeGrainSqla
? TIMEGRAIN_TO_TIMESTAMP[timeGrainSqla]
: 0,
};

if (xAxisType === AxisType.time) {
/**
* Overriding default behavior (false) for time axis regardless of the granilarity.
* Not including this in the initial declaration above so if echarts changes the default
* behavior for other axist types we won't unintentionally override it
*/
xAxis.axisLabel.showMaxLabel = null;
}

let yAxis: any = {
...defaultYAxis,
type: logAxis ? 'log' : 'value',
type: logAxis ? AxisType.log : AxisType.value,
min,
max,
minorTick: { show: true },
Expand Down
7 changes: 7 additions & 0 deletions superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,3 +94,10 @@ export interface EchartsTimeseriesChartProps

export type TimeseriesChartTransformedProps =
EChartTransformedProps<EchartsTimeseriesFormData>;

export enum AxisType {
category = 'category',
value = 'value',
time = 'time',
log = 'log',
}