You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/07/06 17:46:07 UTC
[GitHub] [superset] Antonio-RiveroMartnez opened a new pull request, #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Antonio-RiveroMartnez opened a new pull request, #20627:
URL: https://github.com/apache/superset/pull/20627
### 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
1. Go to a time series line chart with monthly data from the example datasets
2. Chose month time grain
3. Expected results: last month should show on X axis
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] 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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1178088749
@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918919331
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
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 charts 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1203184195
@rusackas Container image not yet published for this PR. Please try again when build is complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918941221
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
Yeah. That's my point. I jumped too fast in my expression.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918928347
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
Yeah. This point makes sense. Another I'm curious why not set `showMaxLabel` option here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918919331
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918932240
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
Actually, you made me realize I call it `showLabel ` and not `showMaxLabel `
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r915804257
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +328,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
},
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 = merge(xAxis, { axisLabel: { showMaxLabel: null } });
Review Comment:
Thanks. Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918931269
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
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). π€
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918500308
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
```suggestion
rotate: xAxisLabelRotation,
```
```suggestion
rotate: xAxisLabelRotation,
showLabel: xAxisType === AxisType.time ? null : false,
```
looks like we can use one line to meet the requirement π€
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] yousoph commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by "yousoph (via GitHub)" <gi...@apache.org>.
yousoph commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1500670369
Oh! Thanks, yeah I don't think we reverted it or fixed the extra timestamp label. We'll take a look
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1203183712
@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1178088727
@rusackas Container image not yet published for this PR. Please try again when build is complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by "Antonio-RiveroMartnez (via GitHub)" <gi...@apache.org>.
Antonio-RiveroMartnez commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1437789662
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1203183698
@rusackas Container image not yet published for this PR. Please try again when build is complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r915804257
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +328,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
},
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 = merge(xAxis, { axisLabel: { showMaxLabel: null } });
Review Comment:
Updated
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1181254521
@stephenLYZ Container image not yet published for this PR. Please try again when build is complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918943160
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
Another suggestion is we can try to set `FEATURE_GENERIC_CHART_AXES=true` to see if it also works.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r919043905
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918936701
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
Thanks! I renamed it to the correct `showMaxLabel`
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
Thanks @stephenLYZ ! I renamed it to the correct `showMaxLabel`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1203184202
@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] villebro commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1436684100
@Antonio-RiveroMartnez I've noticed that this PR has caused time series charts to show weird max labels when the chart is wide enough:
<img width="327" alt="image" src="https://user-images.githubusercontent.com/33317356/220075819-5138e64c-e076-484f-916e-c04c9ed07054.png">
Removing `showMaxLabel: null` resolves it:
<img width="403" alt="image" src="https://user-images.githubusercontent.com/33317356/220075965-b070e0d8-5564-4e67-be06-708c1024460d.png">
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1181254525
@stephenLYZ Ephemeral environment creation failed. Please check the Actions logs for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r919285048
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
@stephenLYZ enabling that flag we get the last month to show but the chart looks off and the format is not correct:
`Changes from this PR:`
<img width="1273" alt="PR" src="https://user-images.githubusercontent.com/38889534/178567647-a19db0e2-e993-4526-9909-29b83e3d49cd.png">
`No changes + Flag enabled:`
<img width="1273" alt="Flag" src="https://user-images.githubusercontent.com/38889534/178567686-df0ceee2-055c-469c-bf2d-524e436d8be7.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1180860288
:label: preset-patch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1203183565
/testenv up FEATURE_GENERIC_CHART_AXES=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1178087820
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] mpostelnicu commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by "mpostelnicu (via GitHub)" <gi...@apache.org>.
mpostelnicu commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1664501610
if possible, please revert this in 2.1.1 because of the extra timestamp label displayed at the end
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1499651993
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)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] codecov[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1176536882
# [Codecov](https://codecov.io/gh/apache/superset/pull/20627?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#20627](https://codecov.io/gh/apache/superset/pull/20627?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (49829e3) into [master](https://codecov.io/gh/apache/superset/commit/f9109583ce1ede0cb2b9f4ad09452bba552a55ce?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f910958) will **decrease** coverage by `0.00%`.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #20627 +/- ##
==========================================
- Coverage 66.82% 66.82% -0.01%
==========================================
Files 1752 1752
Lines 65620 65622 +2
Branches 6938 6939 +1
==========================================
Hits 43853 43853
- Misses 20007 20008 +1
- Partials 1760 1761 +1
```
| Flag | Coverage Ξ | |
|---|---|---|
| javascript | `51.84% <0.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#carryforward-flags-in-the-pull-request-comment) to find out more.
| [Impacted Files](https://codecov.io/gh/apache/superset/pull/20627?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Ξ | |
|---|---|---|
| [...gin-chart-echarts/src/Timeseries/transformProps.ts](https://codecov.io/gh/apache/superset/pull/20627/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3VwZXJzZXQtZnJvbnRlbmQvcGx1Z2lucy9wbHVnaW4tY2hhcnQtZWNoYXJ0cy9zcmMvVGltZXNlcmllcy90cmFuc2Zvcm1Qcm9wcy50cw==) | `53.48% <0.00%> (-1.28%)` | :arrow_down: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/superset/pull/20627?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Ξ = absolute <relative> (impact)`, `ΓΈ = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/superset/pull/20627?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [f910958...49829e3](https://codecov.io/gh/apache/superset/pull/20627?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1203183110
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas merged pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
rusackas merged PR #20627:
URL: https://github.com/apache/superset/pull/20627
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
rusackas commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r920384290
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r920471768
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r919285048
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
@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:`
<img width="1273" alt="PR" src="https://user-images.githubusercontent.com/38889534/178567647-a19db0e2-e993-4526-9909-29b83e3d49cd.png">
`No changes + Flag enabled:`
<img width="1273" alt="Flag" src="https://user-images.githubusercontent.com/38889534/178567686-df0ceee2-055c-469c-bf2d-524e436d8be7.png">
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1178189103
@rusackas Container image not yet published for this PR. Please try again when build is complete.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] github-actions[bot] commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1178189125
@rusackas Ephemeral environment creation failed. Please check the Actions logs for details.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
rusackas commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1178188402
/testenv up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] Antonio-RiveroMartnez commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
Antonio-RiveroMartnez commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r918932240
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +327,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
Review Comment:
Actually, you made me realize I call it `showLabel ` and not `showMaxLabel ` with my second commit. Let me fix that ASAP
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] stephenLYZ commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
stephenLYZ commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1181253929
/testenv up FEATURE_GENERIC_CHART_AXES=true
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] diegomedina248 commented on a diff in pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by GitBox <gi...@apache.org>.
diegomedina248 commented on code in PR #20627:
URL: https://github.com/apache/superset/pull/20627#discussion_r915796625
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts:
##########
@@ -326,13 +328,23 @@ export default function transformProps(
rotate: xAxisLabelRotation,
},
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 = merge(xAxis, { axisLabel: { showMaxLabel: null } });
Review Comment:
do we need the `merge` here? Seems like we're constructing a fresh object above, so maybe `xAxis.axisLabel.showLabel = null` is enough
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [superset] rusackas commented on pull request #20627: fix(chart): Time Series set showMaxLabel as null for time xAxis
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #20627:
URL: https://github.com/apache/superset/pull/20627#issuecomment-1675658591
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org