Skip to content

refactor(explore): convert ControlPanelsContainer to typescript #13221

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

Merged
ktmud merged 18 commits into from
Feb 28, 2021
Merged

refactor(explore): convert ControlPanelsContainer to typescript #13221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
624f994
refactor(explore): convert ControlPanelsContainer to typescript
ktmud Feb 18, 2021
2f00a4f
Fix lint errors
ktmud Feb 18, 2021
d01a7b9
keep provideFormDataToProps to avoid re-render
ktmud Feb 18, 2021
187bcaf
Fix lint errors
ktmud Feb 19, 2021
dd8f2cc
Fix many typing errors
ktmud Feb 20, 2021
aa6be68
Clean up
ktmud Feb 20, 2021
88eac59
Clean up dead import
ktmud Feb 24, 2021
25c509a
move getFormDataFromControls to ts
ktmud Feb 24, 2021
0b1d301
Fix tests
ktmud Feb 24, 2021
1901dde
Fix time range filter value reset
ktmud Feb 24, 2021
fd18840
more clean up for #12782
ktmud Feb 24, 2021
4a3b616
Use reducer to reset control values
ktmud Feb 24, 2021
3520efa
Fix JS error
ktmud Feb 24, 2021
634b84c
Reset control value when initializing explore state
ktmud Feb 24, 2021
ab8dfce
Refactor with clearer typing
ktmud Feb 24, 2021
e4e6172
Simplify the code
ktmud Feb 24, 2021
891fadc
Fix annotation layer test
ktmud Feb 25, 2021
430874c
Better type enforcement
ktmud Feb 26, 2021
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
2 changes: 2 additions & 0 deletions superset-frontend/.eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ module.exports = {
'no-mixed-operators': 0,
'no-multi-assign': 0,
'no-multi-spaces': 0,
'no-nested-ternary': 0,
'no-prototype-builtins': 0,
'no-restricted-properties': 0,
'no-restricted-imports': [
Expand Down Expand Up @@ -226,6 +227,7 @@ module.exports = {
'no-mixed-operators': 0,
'no-multi-assign': 0,
'no-multi-spaces': 0,
'no-nested-ternary': 0,
Copy link
Member Author

@ktmud ktmud Feb 24, 2021

Choose a reason for hiding this comment

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

Disabling as we have seen fairly often nested-ternary in TypeScript extends. It should not be that hard to understand the code with prettier's help.

Copy link
Member Author

@ktmud ktmud Feb 24, 2021

Choose a reason for hiding this comment

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

nested ternary was used in this code:

        const controlName =
          typeof item === 'string'
            ? item
            : item && 'name' in item
            ? item.name
            : null;

Copy link
Member

@etr2460 etr2460 Feb 26, 2021

Choose a reason for hiding this comment

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

Yeah, I'm a fan of nested ternaries when used sparingly. however, couldn't this be simplified to:

        const controlName =
          typeof item === 'string'
            ? item
            : (item || {})?.name ?? null;

or something like that?

Copy link
Member

@etr2460 etr2460 Feb 26, 2021

Choose a reason for hiding this comment

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

Not sure if that's really cleaner, but it doesn't use a nested ternary

Copy link
Member Author

@ktmud ktmud Feb 26, 2021

Choose a reason for hiding this comment

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

'name' in item is needed for type inference so to avoid forced type conversion as item can be of many types, otherwise this could also be as simple as item?.name || item || null.

Here's the error with your approach:

Screen Shot 2021-02-25 at 6 10 55 PM

'no-prototype-builtins': 0,
'no-restricted-properties': 0,
'no-restricted-imports': [
Expand Down
41 changes: 0 additions & 41 deletions ...ress/integration/explore/advanced.test.ts → ...ration/explore/advanced_analytics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,44 +55,3 @@ describe('Advanced analytics', () => {
.contains('1 year');
});
});

describe('Annotations', () => {
beforeEach(() => {
cy.login();
cy.intercept('GET', '/superset/explore_json/**').as('getJson');
cy.intercept('POST', '/superset/explore_json/**').as('postJson');
});

it('Create formula annotation y-axis goal line', () => {
cy.visitChartByName('Num Births Trend');
cy.verifySliceSuccess({ waitAlias: '@postJson' });

cy.get('[data-test=annotation_layers]').within(() => {
cy.get('button').click();
});

cy.get('[data-test="popover-content"]').within(() => {
cy.get('[data-test=annotation-layer-name-header]')
.siblings()
.first()
.within(() => {
cy.get('input').type('Goal line');
});
cy.get('[data-test=annotation-layer-value-header]')
.siblings()
.first()
.within(() => {
cy.get('input').type('y=1400000');
});
cy.get('button').contains('OK').click({ force: true });
});

cy.get('button[data-test="run-query-button"]').click();
cy.verifySliceSuccess({
waitAlias: '@postJson',
chartSelector: 'svg',
});

cy.get('.nv-legend-text').should('have.length', 2);
});
});
59 changes: 59 additions & 0 deletions superset-frontend/cypress-base/cypress/integration/explore/annotations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
* 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.
*/
describe('Annotations', () => {
beforeEach(() => {
cy.login();
cy.intercept('GET', '/superset/explore_json/**').as('getJson');
cy.intercept('POST', '/superset/explore_json/**').as('postJson');
});

it('Create formula annotation y-axis goal line', () => {
cy.visitChartByName('Num Births Trend');
cy.verifySliceSuccess({ waitAlias: '@postJson' });

const layerLabel = 'Goal line';

cy.get('[data-test=annotation_layers] button').click();

cy.get('[data-test="popover-content"]').within(() => {
cy.get('[data-test=annotation-layer-name-header]')
.siblings()
.first()
.within(() => {
cy.get('input').type(layerLabel);
});
cy.get('[data-test=annotation-layer-value-header]')
.siblings()
.first()
.within(() => {
cy.get('input').type('y=1400000');
});
cy.get('button').contains('OK').click();
Copy link
Member Author

@ktmud ktmud Feb 25, 2021

Choose a reason for hiding this comment

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

Moved this test to a separate file. Removed { force: true } to fix the flaky test.

});

cy.get('button[data-test="run-query-button"]').click();
cy.get('[data-test=annotation_layers]').contains(layerLabel);

cy.verifySliceSuccess({
waitAlias: '@postJson',
chartSelector: 'svg',
});
cy.get('.nv-legend-text').should('have.length', 2);
});
});
2 changes: 1 addition & 1 deletion ...rset-frontend/cypress-base/cypress/integration/explore/visualizations/pivot_table.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('Visualization > Pivot Table', () => {
adhoc_filters: [],
groupby: ['name'],
columns: ['state'],
row_limit: 50000,
row_limit: 5000,
Copy link
Member Author

@ktmud ktmud Feb 24, 2021

Choose a reason for hiding this comment

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

Reduce row limit to speed up the test (a little bit)

pandas_aggfunc: 'sum',
pivot_margins: true,
number_format: '.3s',
Expand Down
21 changes: 14 additions & 7 deletions ...omponents/ControlPanelsContainer_spec.jsx → ...omponents/ControlPanelsContainer_spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,17 @@
*/
import React from 'react';
import { shallow } from 'enzyme';
import { getChartControlPanelRegistry, t } from '@superset-ui/core';
import {
DatasourceType,
getChartControlPanelRegistry,
t,
} from '@superset-ui/core';
import { defaultControls } from 'src/explore/store';
import { getFormDataFromControls } from 'src/explore/controlUtils';
import { ControlPanelsContainer } from 'src/explore/components/ControlPanelsContainer';
import {
ControlPanelsContainer,
ControlPanelsContainerProps,
} from 'src/explore/components/ControlPanelsContainer';
import Collapse from 'src/common/components/Collapse';

describe('ControlPanelsContainer', () => {
Expand Down Expand Up @@ -78,15 +85,15 @@ describe('ControlPanelsContainer', () => {
});

function getDefaultProps() {
const controls = defaultControls as ControlPanelsContainerProps['controls'];
return {
datasource_type: 'table',
datasource_type: DatasourceType.Table,
actions: {},
controls: defaultControls,
// Note: default viz_type is table
form_data: getFormDataFromControls(defaultControls),
controls,
form_data: getFormDataFromControls(controls),
isDatasourceMetaLoading: false,
exploreState: {},
};
} as ControlPanelsContainerProps;
}

it('renders ControlPanelSections', () => {
Expand Down
1 change: 0 additions & 1 deletion superset-frontend/src/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
* under the License.
*/
export const DATETIME_WITH_TIME_ZONE = 'YYYY-MM-DD HH:mm:ssZ';

export const TIME_WITH_MS = 'HH:mm:ss.SSS';

export const BOOL_TRUE_DISPLAY = 'True';
Expand Down
8 changes: 1 addition & 7 deletions superset-frontend/src/explore/actions/exploreActions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export const SET_FIELD_VALUE = 'SET_FIELD_VALUE';
export function setControlValue(
controlName: string,
value: any,
validationErrors: any[],
validationErrors?: any[],
) {
return { type: SET_FIELD_VALUE, controlName, value, validationErrors };
}
Expand All @@ -109,11 +109,6 @@ export function setExploreControls(formData: QueryFormData) {
return { type: SET_EXPLORE_CONTROLS, formData };
}

export const REMOVE_CONTROL_PANEL_ALERT = 'REMOVE_CONTROL_PANEL_ALERT';
export function removeControlPanelAlert() {
return { type: REMOVE_CONTROL_PANEL_ALERT };
}
Copy link
Member Author

@ktmud ktmud Feb 18, 2021

Choose a reason for hiding this comment

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

Cleaning up dead code.

alert-deprecated

This is the alert this action is trying to clear, which we are not using anymore.


export const UPDATE_CHART_TITLE = 'UPDATE_CHART_TITLE';
export function updateChartTitle(sliceName: string) {
return { type: UPDATE_CHART_TITLE, sliceName };
Expand Down Expand Up @@ -154,7 +149,6 @@ export const exploreActions = {
saveFaveStar,
setControlValue,
setExploreControls,
removeControlPanelAlert,
updateChartTitle,
createNewSlice,
sliceUpdated,
Expand Down
Loading