ktmud commented on a change in pull request #13221:
URL: https://github.com/apache/superset/pull/13221#discussion_r581793246
##########
File path:
superset-frontend/cypress-base/cypress/integration/explore/visualizations/pivot_table.test.js
##########
@@ -28,7 +28,7 @@ describe('Visualization > Pivot Table', () => {
adhoc_filters: [],
groupby: ['name'],
columns: ['state'],
- row_limit: 50000,
+ row_limit: 5000,
Review comment:
Reduce row limit to speed up the test (a little bit)
##########
File path: superset-frontend/src/explore/components/ControlPanelsContainer.tsx
##########
@@ -80,60 +94,56 @@ const ControlPanelsTabs = styled(Tabs)`
height: 100%;
}
`;
-class ControlPanelsContainer extends React.Component {
+
+class ControlPanelsContainer extends
React.Component<ControlPanelsContainerProps> {
// trigger updates to the component when async plugins load
static contextType = PluginContext;
- constructor(props) {
+ constructor(props: ControlPanelsContainerProps) {
super(props);
-
- this.removeAlert = this.removeAlert.bind(this);
this.renderControl = this.renderControl.bind(this);
this.renderControlPanelSection = this.renderControlPanelSection.bind(this);
}
- componentDidUpdate(prevProps) {
+ componentDidUpdate(prevProps: ControlPanelsContainerProps) {
const {
actions: { setControlValue },
} = this.props;
+ // reset controls using column info (metric, group by, sort by, filter,
etc.)
+ // to default values when datasource changes.
if (this.props.form_data.datasource !== prevProps.form_data.datasource) {
- const defaultValues = [
- 'MetricsControl',
- 'AdhocFilterControl',
- 'TextControl',
- 'SelectControl',
- 'CheckboxControl',
- 'AnnotationLayerControl',
- ];
- Object.entries(this.props.controls).forEach(([controlName, control]) => {
- const { type, default: defaultValue } = control;
- if (defaultValues.includes(type)) {
- setControlValue(controlName, defaultValue);
- }
- });
+ Object.entries(this.props.controls).forEach(
+ ([controlName, controlState]) => {
+ if (
+ // for direct column select controls
+ controlState.valueKey === 'column_name' ||
+ // for all other controls
+ 'columns' in controlState
Review comment:
Instead of check control types, I check whether the control uses column
select.
##########
File path:
superset-frontend/src/explore/components/controls/DateFilterControl/DateFilterControl.tsx
##########
@@ -205,9 +207,9 @@ export default function DateFilterControl(props:
DateFilterLabelProps) {
+--------------+------+----------+--------+----------+-----------+
*/
if (
- frame === 'Common' ||
- frame === 'Calendar' ||
- frame === 'No filter'
+ guessedFrame === 'Common' ||
+ guessedFrame === 'Calendar' ||
+ guessedFrame === 'No filter'
Review comment:
Always use `guessedFrame` to tooltip and actual time range display
because when passing in a new `value` and the popover is not open, the current
frame may be inconsistent with the actual value.
This makes sure `Last week` is always displayed in the pill instead of
tooltip.
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover.jsx
##########
@@ -123,9 +123,6 @@ export default class AdhocMetricEditPopover extends
React.PureComponent {
adhocMetricLabel: this.state.adhocMetric?.getDefaultLabel(),
});
}
- if (prevProps.datasource !== this.props.datasource) {
- this.props.onChange(null);
- }
Review comment:
Clean up the `datasource` prop from adhoc metric popovers as they don't
seem to affect the ability of the controls to reset values when changing
datasource.
@pkdotson do you remember what was these for when you added them?
##########
File path:
superset-frontend/src/explore/components/controls/CollectionControl.jsx
##########
@@ -69,12 +69,6 @@ export default class CollectionControl extends
React.Component {
this.onAdd = this.onAdd.bind(this);
}
- componentDidUpdate(prevProps) {
- if (prevProps.datasource.name !== this.props.datasource.name) {
Review comment:
This one does not work anyway (i.e. change datasource will not reset
Time-series Table columns, and the filters collection in FilterBox) since
previously `props.datasource` is passed as a string.
I don't plan to implement this reset because these control values should not
be reset when changing datasource.
##########
File path: superset-frontend/src/explore/constants.ts
##########
@@ -55,12 +55,7 @@ export const HAVING_OPERATORS = [
OPERATORS['>='],
OPERATORS['<='],
];
-export const MULTI_OPERATORS = new Set([
- OPERATORS.in,
Review comment:
`OPERATORS.in` and `OPERATORS['not in']` are `undefined`, as identified
by converting to typescript, so cleaning them up. `AchocFilter` already always
convert the operator into uppercase.
##########
File path: superset-frontend/src/explore/reducers/exploreReducer.js
##########
@@ -61,6 +62,12 @@ export default function exploreReducer(state = {}, action) {
delete newFormData.time_grain_sqla;
}
}
+ if (
+ action.datasource.id !== state.datasource.id ||
+ action.datasource.type !== state.datasource.type
+ ) {
+ newFormData.time_range = DEFAULT_TIME_RANGE;
+ }
Review comment:
Instead of checking control types, I now check whether a control depends
on datasource columns.
##########
File path: superset-frontend/src/explore/controlUtils/getFormDataFromControls.ts
##########
@@ -0,0 +1,34 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { QueryFormData } from '@superset-ui/core';
+import { ControlStateMapping } from '@superset-ui/chart-controls';
+
+export function getFormDataFromControls(
+ controlsState: ControlStateMapping,
+): QueryFormData {
+ const formData: QueryFormData = {
+ viz_type: 'table',
+ datasource: '',
+ };
+ Object.keys(controlsState).forEach(controlName => {
+ const control = controlsState[controlName];
+ formData[controlName] = control.value;
+ });
+ return formData;
+}
Review comment:
Split functions in `controlUtils` to TypeScript one by one...
##########
File path: superset-frontend/src/explore/components/ControlPanelsContainer.tsx
##########
@@ -80,60 +94,34 @@ const ControlPanelsTabs = styled(Tabs)`
height: 100%;
}
`;
-class ControlPanelsContainer extends React.Component {
+
+class ControlPanelsContainer extends
React.Component<ControlPanelsContainerProps> {
// trigger updates to the component when async plugins load
static contextType = PluginContext;
- constructor(props) {
+ constructor(props: ControlPanelsContainerProps) {
super(props);
-
- this.removeAlert = this.removeAlert.bind(this);
this.renderControl = this.renderControl.bind(this);
this.renderControlPanelSection = this.renderControlPanelSection.bind(this);
}
- componentDidUpdate(prevProps) {
- const {
- actions: { setControlValue },
- } = this.props;
- if (this.props.form_data.datasource !== prevProps.form_data.datasource) {
- const defaultValues = [
- 'MetricsControl',
- 'AdhocFilterControl',
- 'TextControl',
- 'SelectControl',
- 'CheckboxControl',
- 'AnnotationLayerControl',
- ];
- Object.entries(this.props.controls).forEach(([controlName, control]) => {
- const { type, default: defaultValue } = control;
- if (defaultValues.includes(type)) {
- setControlValue(controlName, defaultValue);
- }
- });
- }
- }
Review comment:
Moved the resetting logic to reducers as each of these `setControlValue`
call here will cause another re-rendering.
##########
File path: superset-frontend/src/explore/components/ControlPanelsContainer.tsx
##########
@@ -318,26 +328,19 @@ class ControlPanelsContainer extends React.Component {
}
}
-ControlPanelsContainer.propTypes = propTypes;
-
-function mapStateToProps({ explore }) {
- return {
- alert: explore.controlPanelAlert,
- isDatasourceMetaLoading: explore.isDatasourceMetaLoading,
- controls: explore.controls,
- exploreState: explore,
- };
-}
Review comment:
Move to inline function to use inferred 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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
