You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "michael-s-molina (via GitHub)" <gi...@apache.org> on 2023/06/21 22:15:34 UTC
[GitHub] [superset] michael-s-molina opened a new pull request, #24477: fix: Total calculation in stacked Timeseries charts
michael-s-molina opened a new pull request, #24477:
URL: https://github.com/apache/superset/pull/24477
### SUMMARY
This PR fixes the following bugs:
- The totals where not correctly calculated in stacked Timeseries charts when interacting with the legend
- Double clicking a series was not exclusively selecting it.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
https://github.com/apache/superset/assets/70410625/e44219d3-bb4d-4c5d-a9a6-c95fceca8837
https://github.com/apache/superset/assets/70410625/5baffc10-02b2-4e45-96d1-fd6d329f01e9
### TESTING INSTRUCTIONS
1 - Make sure the totals are correctly calculated for Timeseries charts when interacting with the legend.
2 - Make sure that double clicking on a series exclusively selects it
### ADDITIONAL INFORMATION
- [ ] 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] justinpark commented on a diff in pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24477:
URL: https://github.com/apache/superset/pull/24477#discussion_r1237795842
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -272,15 +235,14 @@ export default function EchartsTimeseries({
// @ts-ignore
const globalModel = echartInstance.getModel();
const model = getModelInfo(params.target, globalModel);
- const seriesCount = globalModel.getSeriesCount();
- const currentSeriesIndices = globalModel.getCurrentSeriesIndices();
if (model) {
const { name } = model;
- if (seriesCount !== currentSeriesIndices.length) {
- handleDoubleClickChange();
- } else {
- handleDoubleClickChange(name);
- }
+ const legendState: LegendState = legendData
+ .map(datum => ({
+ [datum]: datum === name,
+ }))
+ .reduce((previous, current) => ({ ...previous, ...current }));
Review Comment:
If I understand correctly, you don't need to have additional `.map` iteration. just `.reduce` can cover this
```suggestion
.reduce((previous, datum) => ({
...previous,
[datum]: datum == name,
}));
```
--
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] michael-s-molina commented on a diff in pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24477:
URL: https://github.com/apache/superset/pull/24477#discussion_r1238953705
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -52,20 +57,24 @@ export function extractDataTotalValues(
stack: StackType;
percentageThreshold: number;
xAxisCol: string;
+ legendState?: LegendState;
},
): {
totalStackedValues: number[];
thresholdValues: number[];
} {
const totalStackedValues: number[] = [];
const thresholdValues: number[] = [];
- const { stack, percentageThreshold, xAxisCol } = opts;
+ const { stack, percentageThreshold, xAxisCol, legendState } = opts;
if (stack) {
data.forEach(datum => {
const values = Object.keys(datum).reduce((prev, curr) => {
if (curr === xAxisCol) {
return prev;
}
+ if (legendState && !legendState[curr]) {
Review Comment:
If there's no `legendState` then it should skip this statement. It should only enter if `legendState` is defined and the value is not there.
--
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] justinpark commented on a diff in pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24477:
URL: https://github.com/apache/superset/pull/24477#discussion_r1237790600
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -123,12 +123,6 @@ export default function EchartsMixedTimeseries({
const { seriesName, seriesIndex } = props;
handleChange(seriesName, seriesIndex);
},
- mouseout: () => {
- currentSeries.name = '';
- },
- mouseover: params => {
- currentSeries.name = params.seriesName;
Review Comment:
just curious. Is this change related to the calculation bug?
--
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] michael-s-molina commented on a diff in pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24477:
URL: https://github.com/apache/superset/pull/24477#discussion_r1238938152
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/EchartsMixedTimeseries.tsx:
##########
@@ -123,12 +123,6 @@ export default function EchartsMixedTimeseries({
const { seriesName, seriesIndex } = props;
handleChange(seriesName, seriesIndex);
},
- mouseout: () => {
- currentSeries.name = '';
- },
- mouseover: params => {
- currentSeries.name = params.seriesName;
Review Comment:
No. This was just unnecessary code.
--
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 a diff in pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "villebro (via GitHub)" <gi...@apache.org>.
villebro commented on code in PR #24477:
URL: https://github.com/apache/superset/pull/24477#discussion_r1239395231
##########
superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts:
##########
@@ -88,6 +95,8 @@ export interface ChartPropsConfig {
ownState?: JsonObject;
/** Filter state that saved in dashboard */
filterState?: FilterState;
+ /** Legend state */
+ legendState?: LegendState;
Review Comment:
I want to raise the concern of bloating the number of different states we have here: Would it make sense to introduce something more generic, like `chartState` that could be leveraged for transporting other info, too? I assume this isn't the last time we need to ship back info from the plugin to `transformProps`, and having to add a new property can become tedious at some point. Thoughts? Alternatively, let's not do this now, but do a full cleanup when we get around to building the new React-based plugin arch (v2).
--
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] michael-s-molina merged pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina merged PR #24477:
URL: https://github.com/apache/superset/pull/24477
--
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] michael-s-molina commented on a diff in pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "michael-s-molina (via GitHub)" <gi...@apache.org>.
michael-s-molina commented on code in PR #24477:
URL: https://github.com/apache/superset/pull/24477#discussion_r1239792372
##########
superset-frontend/packages/superset-ui-core/src/chart/models/ChartProps.ts:
##########
@@ -88,6 +95,8 @@ export interface ChartPropsConfig {
ownState?: JsonObject;
/** Filter state that saved in dashboard */
filterState?: FilterState;
+ /** Legend state */
+ legendState?: LegendState;
Review Comment:
I totally agree with your concern. I have many other concerns regarding the current plugin architecture but I think the best approach would be to stop amending the flawed design and think about a cleaner v2. One thing I think is really hard with the current structure is to properly test the plugin behaviors.
--
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] justinpark commented on a diff in pull request #24477: fix: Total calculation in stacked Timeseries charts
Posted by "justinpark (via GitHub)" <gi...@apache.org>.
justinpark commented on code in PR #24477:
URL: https://github.com/apache/superset/pull/24477#discussion_r1237791698
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:
##########
@@ -52,20 +57,24 @@ export function extractDataTotalValues(
stack: StackType;
percentageThreshold: number;
xAxisCol: string;
+ legendState?: LegendState;
},
): {
totalStackedValues: number[];
thresholdValues: number[];
} {
const totalStackedValues: number[] = [];
const thresholdValues: number[] = [];
- const { stack, percentageThreshold, xAxisCol } = opts;
+ const { stack, percentageThreshold, xAxisCol, legendState } = opts;
if (stack) {
data.forEach(datum => {
const values = Object.keys(datum).reduce((prev, curr) => {
if (curr === xAxisCol) {
return prev;
}
+ if (legendState && !legendState[curr]) {
Review Comment:
nit: you can shorten this like
```suggestion
if (!legendState?.[curr]) {
```
--
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