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/04/26 20:27:50 UTC
[GitHub] [superset] diegomedina248 commented on a diff in pull request #19841: fix: Table chart column config issue
diegomedina248 commented on code in PR #19841:
URL: https://github.com/apache/superset/pull/19841#discussion_r859113134
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
Review Comment:
since we're modifying these two below, shouldn't we copy these two? Unless shallowly, like `{...state.form_data.column_config}`.
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -148,9 +167,18 @@ export default function exploreReducer(state = {}, action) {
...getControlStateFromControlConfig(controlConfig, state, action.value),
};
+ const column_config = {
+ ...state.controls.column_config,
+ ...(new_column_config && { value: new_column_config }),
+ };
+
const newState = {
...state,
- controls: { ...state.controls, [action.controlName]: control },
+ controls: {
+ ...state.controls,
+ [action.controlName]: control,
Review Comment:
you can use `controlName` directly here
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
const { controlName, value, validationErrors } = action;
new_form_data[controlName] = value;
const vizType = new_form_data.viz_type;
+ // if the controlName is metrics, and the metric column name is updated,
+ // need to update column config as well to keep the previou config.
+ if (controlName === 'metrics' && old_metrics_data && new_column_config) {
+ value.forEach((item, index) => {
+ if (
+ item.label !== old_metrics_data[index].label &&
+ !!new_column_config[old_metrics_data[index].label]
+ ) {
+ new_column_config[item.label] =
+ new_column_config[old_metrics_data[index].label];
+
+ delete new_column_config[old_metrics_data[index].label];
+ }
+ });
+ new_form_data.column_config = new_column_config;
Review Comment:
`new_form_data` is part of the original state, we shouldn't be mutating it
##########
superset-frontend/src/explore/reducers/exploreReducer.js:
##########
@@ -131,11 +131,30 @@ export default function exploreReducer(state = {}, action) {
},
[actions.SET_FIELD_VALUE]() {
const new_form_data = state.form_data;
+ const old_metrics_data = state.form_data.metrics;
+ const new_column_config = state.form_data.column_config;
const { controlName, value, validationErrors } = action;
new_form_data[controlName] = value;
const vizType = new_form_data.viz_type;
+ // if the controlName is metrics, and the metric column name is updated,
+ // need to update column config as well to keep the previou config.
+ if (controlName === 'metrics' && old_metrics_data && new_column_config) {
Review Comment:
consider making this pure, avoiding mutations.
This could result in unexpected bugs in other places of the app if not.
--
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