This is an automated email from the ASF dual-hosted git repository. graceguo pushed a commit to branch scope-selector-modal-v2 in repository https://gitbox.apache.org/repos/asf/incubator-superset.git
commit b48bf317ab45f8feeaa0c35ae0a24d7522a7908e Author: Grace <grace....@airbnb.com> AuthorDate: Tue Oct 29 00:24:27 2019 -0700 fix code review comments (round 1) --- .../dashboard/fixtures/mockDashboardFilters.js | 5 +- .../dashboard/fixtures/mockDashboardLayout.js | 2 +- .../dashboard/reducers/dashboardFilters_spec.js | 21 ++-- superset/assets/src/components/CheckboxChecked.jsx | 28 ----- .../assets/src/components/CheckboxHalfchecked.jsx | 28 ----- superset/assets/src/components/CheckboxIcons.jsx | 40 +++++++ .../assets/src/components/CheckboxUnchecked.jsx | 28 ----- .../components/FilterIndicatorsContainer.jsx | 5 +- .../components/filterscope/FilterFieldTree.jsx | 53 +++++---- .../components/filterscope/FilterScopeModal.jsx | 13 +-- .../components/filterscope/FilterScopeSelector.jsx | 118 ++++++++++----------- .../components/filterscope/FilterScopeTree.jsx | 59 ++++++----- .../filterscope/renderFilterFieldTreeNodes.jsx | 9 +- .../filterscope/renderFilterScopeTreeNodes.jsx | 64 ++++++----- .../src/dashboard/containers/FilterScope.jsx | 1 - .../src/dashboard/reducers/dashboardFilters.js | 14 +++ .../src/dashboard/stylesheets/dashboard.less | 4 +- .../stylesheets/filter-scope-selector.less | 15 +-- .../src/dashboard/util/activeDashboardFilters.js | 10 +- .../src/dashboard/util/dashboardFiltersColorMap.js | 2 +- .../src/dashboard/util/getDashboardFilterKey.js | 8 +- .../src/dashboard/util/getFilterFieldNodesTree.js | 6 +- .../src/dashboard/util/getFilterScopeNodesTree.js | 6 +- .../dashboard/util/getFilterScopeParentNodes.js | 2 +- superset/assets/src/dashboard/util/propShapes.jsx | 24 +++++ .../src/visualizations/FilterBox/FilterBox.jsx | 2 +- 26 files changed, 294 insertions(+), 273 deletions(-) diff --git a/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardFilters.js b/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardFilters.js index e72e13e..7f13db7 100644 --- a/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardFilters.js +++ b/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardFilters.js @@ -17,6 +17,7 @@ * under the License. */ import { filterId } from './mockSliceEntities'; +import { DASHBOARD_FILTER_SCOPE_GLOBAL } from '../../../../src/dashboard/reducers/dashboardFilters'; export const emptyFilters = {}; @@ -33,7 +34,9 @@ export const dashboardFilters = { 'ROW-l6PrlhwSjh', 'CHART-rwDfbGqeEn', ], - scope: 'ROOT_ID', + scopes: { + region: DASHBOARD_FILTER_SCOPE_GLOBAL, + }, isDateFilter: false, isInstantFilter: true, columns: { diff --git a/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardLayout.js b/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardLayout.js index aad4445..3267c5c 100644 --- a/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardLayout.js +++ b/superset/assets/spec/javascripts/dashboard/fixtures/mockDashboardLayout.js @@ -204,6 +204,6 @@ export const filterComponent = { chartId: filterId, width: 3, height: 10, - chartName: 'Filter', + sliceName: 'Filter', }, }; diff --git a/superset/assets/spec/javascripts/dashboard/reducers/dashboardFilters_spec.js b/superset/assets/spec/javascripts/dashboard/reducers/dashboardFilters_spec.js index 34a09b5..e234248 100644 --- a/superset/assets/spec/javascripts/dashboard/reducers/dashboardFilters_spec.js +++ b/superset/assets/spec/javascripts/dashboard/reducers/dashboardFilters_spec.js @@ -22,7 +22,9 @@ import { REMOVE_FILTER, CHANGE_FILTER, } from '../../../../src/dashboard/actions/dashboardFilters'; -import dashboardFiltersReducer from '../../../../src/dashboard/reducers/dashboardFilters'; +import dashboardFiltersReducer, { + DASHBOARD_FILTER_SCOPE_GLOBAL, +} from '../../../../src/dashboard/reducers/dashboardFilters'; import { emptyFilters, dashboardFilters, @@ -33,10 +35,8 @@ import { column, } from '../fixtures/mockSliceEntities'; import { filterComponent } from '../fixtures/mockDashboardLayout'; -import { DASHBOARD_ROOT_ID } from '../../../../src/dashboard/util/constants'; -// disable broken unit tests by now, will fix it in another PR -xdescribe('dashboardFilters reducer', () => { +describe('dashboardFilters reducer', () => { const form_data = sliceEntitiesForDashboard.slices[filterId].form_data; const component = filterComponent; const directPathToFilter = (component.parents || []).slice(); @@ -55,7 +55,7 @@ xdescribe('dashboardFilters reducer', () => { chartId: filterId, componentId: component.id, directPathToFilter, - scope: 'ROOT_ID', + filterName: component.meta.sliceName, isDateFilter: false, isInstantFilter: !!form_data.instant_filtering, columns: { @@ -64,6 +64,9 @@ xdescribe('dashboardFilters reducer', () => { labels: { [column]: column, }, + scopes: { + [column]: DASHBOARD_FILTER_SCOPE_GLOBAL, + }, }, }); }); @@ -84,7 +87,6 @@ xdescribe('dashboardFilters reducer', () => { chartId: filterId, componentId: component.id, directPathToFilter, - scopes: {}, isDateFilter: false, isInstantFilter: !!form_data.instant_filtering, columns: { @@ -94,6 +96,9 @@ xdescribe('dashboardFilters reducer', () => { labels: { [column]: column, }, + scopes: { + [column]: DASHBOARD_FILTER_SCOPE_GLOBAL, + }, }, }); }); @@ -114,7 +119,6 @@ xdescribe('dashboardFilters reducer', () => { chartId: filterId, componentId: component.id, directPathToFilter, - scope: DASHBOARD_ROOT_ID, isDateFilter: false, isInstantFilter: !!form_data.instant_filtering, columns: { @@ -124,6 +128,9 @@ xdescribe('dashboardFilters reducer', () => { labels: { [column]: column, }, + scopes: { + [column]: DASHBOARD_FILTER_SCOPE_GLOBAL, + }, }, }); }); diff --git a/superset/assets/src/components/CheckboxChecked.jsx b/superset/assets/src/components/CheckboxChecked.jsx deleted file mode 100644 index e88aad0..0000000 --- a/superset/assets/src/components/CheckboxChecked.jsx +++ /dev/null @@ -1,28 +0,0 @@ -/** - * 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 React from 'react'; - -export default function CheckboxChecked() { - return ( - <svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M16 0H2C0.89 0 0 0.9 0 2V16C0 17.1 0.89 18 2 18H16C17.11 18 18 17.1 18 16V2C18 0.9 17.11 0 16 0Z" fill="#00A699" /> - <path d="M7 14L2 9L3.41 7.59L7 11.17L14.59 3.58L16 5L7 14Z" fill="white" /> - </svg> - ); -} diff --git a/superset/assets/src/components/CheckboxHalfchecked.jsx b/superset/assets/src/components/CheckboxHalfchecked.jsx deleted file mode 100644 index 7122e7c..0000000 --- a/superset/assets/src/components/CheckboxHalfchecked.jsx +++ /dev/null @@ -1,28 +0,0 @@ -/** - * 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 React from 'react'; - -export default function CheckboxHalfchecked() { - return ( - <svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M16 0H2C0.9 0 0 0.9 0 2V16C0 17.1 0.9 18 2 18H16C17.1 18 18 17.1 18 16V2C18 0.9 17.1 0 16 0Z" fill="#999999" /> - <path d="M14 10H4V8H14V10Z" fill="white" /> - </svg> - ); -} diff --git a/superset/assets/src/components/CheckboxIcons.jsx b/superset/assets/src/components/CheckboxIcons.jsx new file mode 100644 index 0000000..f26e752 --- /dev/null +++ b/superset/assets/src/components/CheckboxIcons.jsx @@ -0,0 +1,40 @@ +/** + * 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 React from 'react'; + +export const CheckboxChecked = () => ( + <svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"> + <path d="M16 0H2C0.89 0 0 0.9 0 2V16C0 17.1 0.89 18 2 18H16C17.11 18 18 17.1 18 16V2C18 0.9 17.11 0 16 0Z" fill="#00A699" /> + <path d="M7 14L2 9L3.41 7.59L7 11.17L14.59 3.58L16 5L7 14Z" fill="white" /> + </svg> +); + +export const CheckboxHalfChecked = () => ( + <svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"> + <path d="M16 0H2C0.9 0 0 0.9 0 2V16C0 17.1 0.9 18 2 18H16C17.1 18 18 17.1 18 16V2C18 0.9 17.1 0 16 0Z" fill="#999999" /> + <path d="M14 10H4V8H14V10Z" fill="white" /> + </svg> +); + +export const CheckboxUnchecked = () => ( + <svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"> + <path d="M16 0H2C0.9 0 0 0.9 0 2V16C0 17.1 0.9 18 2 18H16C17.1 18 18 17.1 18 16V2C18 0.9 17.1 0 16 0Z" fill="#CCCCCC" /> + <path d="M16 2V16H2V2H16V2Z" fill="white" /> + </svg> +); diff --git a/superset/assets/src/components/CheckboxUnchecked.jsx b/superset/assets/src/components/CheckboxUnchecked.jsx deleted file mode 100644 index 0153789..0000000 --- a/superset/assets/src/components/CheckboxUnchecked.jsx +++ /dev/null @@ -1,28 +0,0 @@ -/** - * 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 React from 'react'; - -export default function CheckboxUnchecked() { - return ( - <svg width="18" height="18" viewBox="0 0 18 18" fill="none" xmlns="http://www.w3.org/2000/svg"> - <path d="M16 0H2C0.9 0 0 0.9 0 2V16C0 17.1 0.9 18 2 18H16C17.1 18 18 17.1 18 16V2C18 0.9 17.1 0 16 0Z" fill="#CCCCCC" /> - <path d="M16 2V16H2V2H16V2Z" fill="white" /> - </svg> - ); -} diff --git a/superset/assets/src/dashboard/components/FilterIndicatorsContainer.jsx b/superset/assets/src/dashboard/components/FilterIndicatorsContainer.jsx index 6c8b6cf..f287108 100644 --- a/superset/assets/src/dashboard/components/FilterIndicatorsContainer.jsx +++ b/superset/assets/src/dashboard/components/FilterIndicatorsContainer.jsx @@ -90,7 +90,10 @@ export default class FilterIndicatorsContainer extends React.PureComponent { !filterImmuneSlices.includes(currentChartId) ) { Object.keys(columns).forEach(name => { - const colorMapKey = getDashboardFilterKey(chartId, name); + const colorMapKey = getDashboardFilterKey({ + chartId, + column: name, + }); const directPathToLabel = directPathToFilter.slice(); directPathToLabel.push(`LABEL-${name}`); const indicator = { diff --git a/superset/assets/src/dashboard/components/filterscope/FilterFieldTree.jsx b/superset/assets/src/dashboard/components/filterscope/FilterFieldTree.jsx index dd76e96..a1b7581 100644 --- a/superset/assets/src/dashboard/components/filterscope/FilterFieldTree.jsx +++ b/superset/assets/src/dashboard/components/filterscope/FilterFieldTree.jsx @@ -21,26 +21,46 @@ import PropTypes from 'prop-types'; import CheckboxTree from 'react-checkbox-tree'; import 'react-checkbox-tree/lib/react-checkbox-tree.css'; -import CheckboxChecked from '../../../components/CheckboxChecked'; -import CheckboxUnchecked from '../../../components/CheckboxUnchecked'; -import CheckboxHalfchecked from '../../../components/CheckboxHalfchecked'; +import { + CheckboxChecked, + CheckboxUnchecked, + CheckboxHalfChecked, +} from '../../../components/CheckboxIcons'; import renderFilterFieldTreeNodes from './renderFilterFieldTreeNodes'; +import { filterScopeSelectorTreeNodePropShape } from '../../util/propShapes'; const propTypes = { activeKey: PropTypes.string.isRequired, - nodes: PropTypes.arrayOf(PropTypes.object).isRequired, - checked: PropTypes.arrayOf(PropTypes.string).isRequired, - expanded: PropTypes.arrayOf(PropTypes.string).isRequired, + nodes: PropTypes.arrayOf(filterScopeSelectorTreeNodePropShape).isRequired, + checked: PropTypes.arrayOf( + PropTypes.oneOfType([PropTypes.number, PropTypes.string]), + ).isRequired, + expanded: PropTypes.arrayOf( + PropTypes.oneOfType([PropTypes.number, PropTypes.string]), + ).isRequired, onCheck: PropTypes.func.isRequired, onExpand: PropTypes.func.isRequired, onClick: PropTypes.func.isRequired, }; +const FILTER_FIELD_CHECKBOX_TREE_ICONS = { + check: <CheckboxChecked />, + uncheck: <CheckboxUnchecked />, + halfCheck: <CheckboxHalfChecked />, + expandClose: <span className="rct-icon rct-icon-expand-close" />, + expandOpen: <span className="rct-icon rct-icon-expand-open" />, + expandAll: <span className="rct-icon rct-icon-expand-all" />, + collapseAll: <span className="rct-icon rct-icon-collapse-all" />, + parentClose: <span className="rct-icon rct-icon-parent-close" />, + parentOpen: <span className="rct-icon rct-icon-parent-open" />, + leaf: <span className="rct-icon rct-icon-leaf" />, +}; + export default function FilterFieldTree({ - activeKey, - nodes, - checked, - expanded, + activeKey = '', + nodes = [], + checked = [], + expanded = [], onClick, onCheck, onExpand, @@ -56,18 +76,7 @@ export default function FilterFieldTree({ onClick={onClick} onCheck={onCheck} onExpand={onExpand} - icons={{ - check: <CheckboxChecked />, - uncheck: <CheckboxUnchecked />, - halfCheck: <CheckboxHalfchecked />, - expandClose: <span className="rct-icon rct-icon-expand-close" />, - expandOpen: <span className="rct-icon rct-icon-expand-open" />, - expandAll: <span className="rct-icon rct-icon-expand-all" />, - collapseAll: <span className="rct-icon rct-icon-collapse-all" />, - parentClose: <span className="rct-icon rct-icon-parent-close" />, - parentOpen: <span className="rct-icon rct-icon-parent-open" />, - leaf: <span className="rct-icon rct-icon-leaf" />, - }} + icons={FILTER_FIELD_CHECKBOX_TREE_ICONS} /> ); } diff --git a/superset/assets/src/dashboard/components/filterscope/FilterScopeModal.jsx b/superset/assets/src/dashboard/components/filterscope/FilterScopeModal.jsx index bc20185..ba2b153 100644 --- a/superset/assets/src/dashboard/components/filterscope/FilterScopeModal.jsx +++ b/superset/assets/src/dashboard/components/filterscope/FilterScopeModal.jsx @@ -30,24 +30,21 @@ export default class FilterScopeModal extends React.PureComponent { constructor(props) { super(props); - this.modal = null; + this.modal = React.createRef(); this.close = this.close.bind(this); - this.setModalRef = this.setModalRef.bind(this); - } - - setModalRef(ref) { - this.modal = ref; } close() { - this.modal.close(); + if (this.modal) { + this.modal.current.close(); + } } render() { return ( <ModalTrigger dialogClassName="filter-scope-modal" - ref={this.setModalRef} + ref={this.modal} triggerNode={this.props.triggerNode} modalBody={ <div className="dashboard-modal filter-scope"> diff --git a/superset/assets/src/dashboard/components/filterscope/FilterScopeSelector.jsx b/superset/assets/src/dashboard/components/filterscope/FilterScopeSelector.jsx index 79256ac..855e66d 100644 --- a/superset/assets/src/dashboard/components/filterscope/FilterScopeSelector.jsx +++ b/superset/assets/src/dashboard/components/filterscope/FilterScopeSelector.jsx @@ -31,7 +31,7 @@ import getRevertedFilterScope from '../../util/getRevertedFilterScope'; import FilterScopeTree from './FilterScopeTree'; import FilterFieldTree from './FilterFieldTree'; import { - getDashboardFilterByKey, + getChartIdAndColumnFromFilterKey, getDashboardFilterKey, } from '../../util/getDashboardFilterKey'; @@ -40,7 +40,6 @@ const propTypes = { layout: PropTypes.object.isRequired, filterImmuneSlices: PropTypes.arrayOf(PropTypes.number).isRequired, filterImmuneSliceFields: PropTypes.object.isRequired, - setDirectPathToChild: PropTypes.func.isRequired, onCloseModal: PropTypes.func.isRequired, }; @@ -69,12 +68,14 @@ export default class FilterScopeSelector extends React.PureComponent { }); }); - this.defaultFilterKey = Object.keys(filterFieldNodes).length - ? filterFieldNodes[0].children[0].value - : ''; + if (filterFieldNodes.length && filterFieldNodes[0].children) { + this.defaultFilterKey = filterFieldNodes[0].children[0].value; + } const checkedFilterFields = [this.defaultFilterKey]; // expand defaultFilterKey - const [chartId] = getDashboardFilterByKey(this.defaultFilterKey); + const { chartId } = getChartIdAndColumnFromFilterKey( + this.defaultFilterKey, + ); const expandedFilterIds = [chartId]; // display checkbox tree of whole dashboard layout @@ -82,13 +83,23 @@ export default class FilterScopeSelector extends React.PureComponent { (map, { chartId: filterId, columns }) => { const filterScopeByChartId = Object.keys(columns).reduce( (mapByChartId, columnName) => { - const filterKey = getDashboardFilterKey(filterId, columnName); + const filterKey = getDashboardFilterKey({ + chartId: filterId, + column: columnName, + }); const nodes = getFilterScopeNodesTree({ components: layout, isSingleEditMode: true, checkedFilterFields, selectedChartId: filterId, }); + const checked = getCurrentScopeChartIds({ + scopeComponentIds: ['ROOT_ID'], // dashboardFilters[chartId].scopes[columnName], + filterField: columnName, + filterImmuneSlices, + filterImmuneSliceFields, + components: layout, + }); const expanded = getFilterScopeParentNodes(nodes, 1); return { ...mapByChartId, @@ -97,13 +108,7 @@ export default class FilterScopeSelector extends React.PureComponent { nodes, // filtered nodes in display if searchText is not empty nodesFiltered: nodes.slice(), - checked: getCurrentScopeChartIds({ - scopeComponentIds: ['ROOT_ID'], // dashboardFilters[chartId].scopes[columnName], - filterField: columnName, - filterImmuneSlices, - filterImmuneSliceFields, - components: layout, - }), + checked, expanded, }, }; @@ -141,14 +146,13 @@ export default class FilterScopeSelector extends React.PureComponent { this.onCheckFilterScope = this.onCheckFilterScope.bind(this); this.onExpandFilterScope = this.onExpandFilterScope.bind(this); this.onSearchInputChange = this.onSearchInputChange.bind(this); - this.onClickFilterField = this.onClickFilterField.bind(this); this.onCheckFilterField = this.onCheckFilterField.bind(this); this.onExpandFilterField = this.onExpandFilterField.bind(this); this.onClose = this.onClose.bind(this); this.onSave = this.onSave.bind(this); } - onCheckFilterScope(checked) { + onCheckFilterScope(checked = []) { const { activeKey, filterScopeMap, @@ -159,7 +163,7 @@ export default class FilterScopeSelector extends React.PureComponent { if (isSingleEditMode) { const updatedEntry = { ...filterScopeMap[activeKey], - checked: checked.map(c => JSON.parse(c)), + checked: checked.map(c => parseInt(c, 10)), }; this.setState(() => ({ @@ -191,7 +195,7 @@ export default class FilterScopeSelector extends React.PureComponent { } } - onExpandFilterScope(expanded) { + onExpandFilterScope(expanded = []) { const { activeKey, filterScopeMap } = this.state; const updatedEntry = { ...filterScopeMap[activeKey], @@ -205,11 +209,7 @@ export default class FilterScopeSelector extends React.PureComponent { })); } - onClickFilterField(filterField) { - this.onChangeFilterField(filterField.value); - } - - onCheckFilterField(checkedFilterFields) { + onCheckFilterField(checkedFilterFields = []) { const { layout } = this.props; const { isSingleEditMode, filterScopeMap } = this.state; const nodes = getFilterScopeNodesTree({ @@ -232,7 +232,7 @@ export default class FilterScopeSelector extends React.PureComponent { ...filterScopeMap, [activeKey]: { nodes, - nodesFiltered: nodes.slice(), + nodesFiltered: [...nodes], checked: [...checkedChartIdSet], expanded: getFilterScopeParentNodes(nodes, 1), }, @@ -240,7 +240,7 @@ export default class FilterScopeSelector extends React.PureComponent { })); } - onExpandFilterField(expandedFilterIds) { + onExpandFilterField(expandedFilterIds = []) { this.setState(() => ({ expandedFilterIds, })); @@ -250,7 +250,8 @@ export default class FilterScopeSelector extends React.PureComponent { this.setState({ searchText: e.target.value }, this.filterTree); } - onChangeFilterField(nextActiveKey) { + onChangeFilterField(filterField = {}) { + const nextActiveKey = filterField.value; const { isSingleEditMode, activeKey: currentActiveKey, @@ -339,32 +340,30 @@ export default class FilterScopeSelector extends React.PureComponent { }, }; }); - - return; - } - - const updater = prevState => { - const { activeKey, filterScopeMap } = prevState; - const nodesFiltered = filterScopeMap[activeKey].nodes.reduce( - this.filterNodes, - [], - ); - const updatedEntry = { - ...filterScopeMap[activeKey], - nodesFiltered, - }; - return { - filterScopeMap: { - ...filterScopeMap, - [activeKey]: updatedEntry, - }, + } else { + const updater = prevState => { + const { activeKey, filterScopeMap } = prevState; + const nodesFiltered = filterScopeMap[activeKey].nodes.reduce( + this.filterNodes, + [], + ); + const updatedEntry = { + ...filterScopeMap[activeKey], + nodesFiltered, + }; + return { + filterScopeMap: { + ...filterScopeMap, + [activeKey]: updatedEntry, + }, + }; }; - }; - this.setState(updater); + this.setState(updater); + } } - filterNodes(filtered, node) { + filterNodes(filtered = [], node = {}) { const { searchText } = this.state; const children = (node.children || []).reduce(this.filterNodes, []); @@ -394,7 +393,7 @@ export default class FilterScopeSelector extends React.PureComponent { nodes={filterFieldNodes} checked={checkedFilterFields} expanded={expandedFilterIds} - onClick={this.onClickFilterField} + onClick={this.onChangeFilterField} onCheck={this.onCheckFilterField} onExpand={this.onExpandFilterField} /> @@ -409,16 +408,16 @@ export default class FilterScopeSelector extends React.PureComponent { searchText, } = this.state; - const [chartId] = isSingleEditMode - ? getDashboardFilterByKey(activeKey) - : [0]; + const selectedFilterId = isSingleEditMode + ? getChartIdAndColumnFromFilterKey(activeKey).chartId + : 0; return ( <React.Fragment> <input className={cx('filter-text scope-search', { 'multi-edit-mode': !isSingleEditMode, })} - placeholder="Search..." + placeholder={t('Search...')} type="text" value={searchText} onChange={this.onSearchInputChange} @@ -429,8 +428,9 @@ export default class FilterScopeSelector extends React.PureComponent { expanded={filterScopeMap[activeKey].expanded} onCheck={this.onCheckFilterScope} onExpand={this.onExpandFilterScope} - // hide checkbox for selected filter field itself - selectedFilterId={chartId} + // pass selectedFilterId prop to FilterScopeTree component, + // to hide checkbox for selected filter field itself + selectedFilterId={selectedFilterId} /> </React.Fragment> ); @@ -459,7 +459,7 @@ export default class FilterScopeSelector extends React.PureComponent { const currentFilterLabels = [] .concat(isSingleValue ? activeKey : JSON.parse(activeKey)) .map(key => { - const [chartId, column] = getDashboardFilterByKey(key); + const { chartId, column } = getChartIdAndColumnFromFilterKey(key); return dashboardFilters[chartId].labels[column] || column; }); @@ -480,9 +480,9 @@ export default class FilterScopeSelector extends React.PureComponent { </div> </div> - {!showSelector && <div>There is no filter in this dashboard</div>} - - {showSelector && ( + {!showSelector ? ( + <div>{t('There are no filter in this dashboard')}</div> + ) : ( <div className="filters-scope-selector"> <div className={cx('filter-field-pane', { diff --git a/superset/assets/src/dashboard/components/filterscope/FilterScopeTree.jsx b/superset/assets/src/dashboard/components/filterscope/FilterScopeTree.jsx index 9e869a5..08769ba 100644 --- a/superset/assets/src/dashboard/components/filterscope/FilterScopeTree.jsx +++ b/superset/assets/src/dashboard/components/filterscope/FilterScopeTree.jsx @@ -21,51 +21,62 @@ import PropTypes from 'prop-types'; import CheckboxTree from 'react-checkbox-tree'; import 'react-checkbox-tree/lib/react-checkbox-tree.css'; -import CheckboxChecked from '../../../components/CheckboxChecked'; -import CheckboxUnchecked from '../../../components/CheckboxUnchecked'; -import CheckboxHalfchecked from '../../../components/CheckboxHalfchecked'; +import { + CheckboxChecked, + CheckboxUnchecked, + CheckboxHalfChecked, +} from '../../../components/CheckboxIcons'; import renderFilterScopeTreeNodes from './renderFilterScopeTreeNodes'; +import { filterScopeSelectorTreeNodePropShape } from '../../util/propShapes'; const propTypes = { - nodes: PropTypes.arrayOf(PropTypes.object).isRequired, - checked: PropTypes.arrayOf(PropTypes.string).isRequired, - expanded: PropTypes.arrayOf(PropTypes.string).isRequired, + nodes: PropTypes.arrayOf(filterScopeSelectorTreeNodePropShape).isRequired, + checked: PropTypes.arrayOf( + PropTypes.oneOfType([PropTypes.number, PropTypes.string]), + ).isRequired, + expanded: PropTypes.arrayOf( + PropTypes.oneOfType([PropTypes.number, PropTypes.string]), + ).isRequired, onCheck: PropTypes.func.isRequired, onExpand: PropTypes.func.isRequired, selectedFilterId: PropTypes.number.isRequired, }; +const NOOP = () => {}; + +const FILTER_SCOPE_CHECKBOX_TREE_ICONS = { + check: <CheckboxChecked />, + uncheck: <CheckboxUnchecked />, + halfCheck: <CheckboxHalfChecked />, + expandClose: <span className="rct-icon rct-icon-expand-close" />, + expandOpen: <span className="rct-icon rct-icon-expand-open" />, + expandAll: <span className="rct-icon rct-icon-expand-all" />, + collapseAll: <span className="rct-icon rct-icon-collapse-all" />, + parentClose: <span className="rct-icon rct-icon-parent-close" />, + parentOpen: <span className="rct-icon rct-icon-parent-open" />, + leaf: <span className="rct-icon rct-icon-leaf" />, +}; + export default function FilterScopeTree({ - nodes, - checked, - expanded, + nodes = [], + checked = [], + expanded = [], onCheck, onExpand, - selectedFilterId, + selectedFilterId = 0, }) { return ( <CheckboxTree showExpandAll expandOnClick showNodeIcon={false} - nodes={renderFilterScopeTreeNodes(nodes, selectedFilterId)} + nodes={renderFilterScopeTreeNodes({ nodes, selectedFilterId })} checked={checked} expanded={expanded} onCheck={onCheck} onExpand={onExpand} - onClick={() => {}} - icons={{ - check: <CheckboxChecked />, - uncheck: <CheckboxUnchecked />, - halfCheck: <CheckboxHalfchecked />, - expandClose: <span className="rct-icon rct-icon-expand-close" />, - expandOpen: <span className="rct-icon rct-icon-expand-open" />, - expandAll: <span className="rct-icon rct-icon-expand-all" />, - collapseAll: <span className="rct-icon rct-icon-collapse-all" />, - parentClose: <span className="rct-icon rct-icon-parent-close" />, - parentOpen: <span className="rct-icon rct-icon-parent-open" />, - leaf: <span className="rct-icon rct-icon-leaf" />, - }} + onClick={NOOP} + icons={FILTER_SCOPE_CHECKBOX_TREE_ICONS} /> ); } diff --git a/superset/assets/src/dashboard/components/filterscope/renderFilterFieldTreeNodes.jsx b/superset/assets/src/dashboard/components/filterscope/renderFilterFieldTreeNodes.jsx index b3866a9..ac9e9b1 100644 --- a/superset/assets/src/dashboard/components/filterscope/renderFilterFieldTreeNodes.jsx +++ b/superset/assets/src/dashboard/components/filterscope/renderFilterFieldTreeNodes.jsx @@ -21,11 +21,10 @@ import React from 'react'; import FilterFieldItem from './FilterFieldItem'; import { getFilterColorMap } from '../../util/dashboardFiltersColorMap'; -export default function renderFilterFieldTreeNodes({ nodes, activeKey }) { - if (nodes.length === 0) { - return []; - } - +export default function renderFilterFieldTreeNodes({ + nodes = [], + activeKey = '', +}) { return nodes.map(node => ({ ...node, children: node.children.map(child => { diff --git a/superset/assets/src/dashboard/components/filterscope/renderFilterScopeTreeNodes.jsx b/superset/assets/src/dashboard/components/filterscope/renderFilterScopeTreeNodes.jsx index 5d0fbf1..a4982b7 100644 --- a/superset/assets/src/dashboard/components/filterscope/renderFilterScopeTreeNodes.jsx +++ b/superset/assets/src/dashboard/components/filterscope/renderFilterScopeTreeNodes.jsx @@ -18,43 +18,53 @@ */ import React from 'react'; import cx from 'classnames'; +import { t } from '@superset-ui/translation'; -export default function renderFilterScopeTreeNodes(nodes, selectedFilterId) { - if (nodes.length === 0) { - return []; - } - - function traverse(currentNode) { - if (!currentNode) { - return null; - } +import { DASHBOARD_ROOT_TYPE } from '../../util/componentTypes'; - const { label, type, children } = currentNode; - if (children && children.length) { - const updatedChildren = children.map(child => traverse(child)); - return { - ...currentNode, - label: ( - <a className={`filter-scope-type ${type.toLowerCase()}`}>{label}</a> - ), - children: updatedChildren, - }; - } +function traverse({ currentNode, selectedFilterId }) { + if (!currentNode) { + return null; + } - const { value } = currentNode; + const { label, type, children } = currentNode; + if (children && children.length) { + const updatedChildren = children.map(child => + traverse({ currentNode: child, selectedFilterId }), + ); return { ...currentNode, label: ( - <a - className={cx(`filter-scope-type ${type.toLowerCase()}`, { - 'selected-filter': selectedFilterId === value, - })} - > + <a className={`filter-scope-type ${type.toLowerCase()}`}> + {type !== DASHBOARD_ROOT_TYPE && ( + <span className="type-indicator">{t(type)}</span> + )} {label} </a> ), + children: updatedChildren, }; } - return nodes.map(node => traverse(node)); + const { value } = currentNode; + return { + ...currentNode, + label: ( + <a + className={cx(`filter-scope-type ${type.toLowerCase()}`, { + 'selected-filter': selectedFilterId === value, + })} + > + <span className="type-indicator">{t(type)}</span> + {label} + </a> + ), + }; +} + +export default function renderFilterScopeTreeNodes({ + nodes = [], + selectedFilterId = 0, +}) { + return nodes.map(node => traverse({ currentNode: node, selectedFilterId })); } diff --git a/superset/assets/src/dashboard/containers/FilterScope.jsx b/superset/assets/src/dashboard/containers/FilterScope.jsx index 6758ce5..f88d665 100644 --- a/superset/assets/src/dashboard/containers/FilterScope.jsx +++ b/superset/assets/src/dashboard/containers/FilterScope.jsx @@ -29,7 +29,6 @@ function mapStateToProps({ dashboardLayout, dashboardFilters, dashboardInfo }) { filterImmuneSliceFields: dashboardInfo.metadata.filterImmuneSliceFields || {}, layout: dashboardLayout.present, - // closeModal: ownProps.onCloseModal, }; } diff --git a/superset/assets/src/dashboard/reducers/dashboardFilters.js b/superset/assets/src/dashboard/reducers/dashboardFilters.js index 1525462..224b50b 100644 --- a/superset/assets/src/dashboard/reducers/dashboardFilters.js +++ b/superset/assets/src/dashboard/reducers/dashboardFilters.js @@ -27,6 +27,12 @@ import { TIME_RANGE } from '../../visualizations/FilterBox/FilterBox'; import getFilterConfigsFromFormdata from '../util/getFilterConfigsFromFormdata'; import { buildFilterColorMap } from '../util/dashboardFiltersColorMap'; import { buildActiveFilters } from '../util/activeDashboardFilters'; +import { DASHBOARD_ROOT_ID } from '../util/constants'; + +export const DASHBOARD_FILTER_SCOPE_GLOBAL = { + scope: [DASHBOARD_ROOT_ID], + immune: [], +}; export const dashboardFilter = { chartId: 0, @@ -45,6 +51,13 @@ export default function dashboardFiltersReducer(dashboardFilters = {}, action) { [ADD_FILTER]() { const { chartId, component, form_data } = action; const { columns, labels } = getFilterConfigsFromFormdata(form_data); + const scopes = Object.keys(columns).reduce( + (map, column) => ({ + ...map, + [column]: DASHBOARD_FILTER_SCOPE_GLOBAL, + }), + {}, + ); const directPathToFilter = component ? (component.parents || []).slice().concat(component.id) : []; @@ -57,6 +70,7 @@ export default function dashboardFiltersReducer(dashboardFilters = {}, action) { directPathToFilter, columns, labels, + scopes, isInstantFilter: !!form_data.instant_filtering, isDateFilter: Object.keys(columns).includes(TIME_RANGE), }; diff --git a/superset/assets/src/dashboard/stylesheets/dashboard.less b/superset/assets/src/dashboard/stylesheets/dashboard.less index fd9288d..cdf11f7 100644 --- a/superset/assets/src/dashboard/stylesheets/dashboard.less +++ b/superset/assets/src/dashboard/stylesheets/dashboard.less @@ -183,8 +183,8 @@ body { .dashboard-modal.delete { .btn.btn-primary { - background: @pink !important; - border-color: @pink !important; + background: @pink; + border-color: @pink; } } } diff --git a/superset/assets/src/dashboard/stylesheets/filter-scope-selector.less b/superset/assets/src/dashboard/stylesheets/filter-scope-selector.less index d57d6de..cef4083 100644 --- a/superset/assets/src/dashboard/stylesheets/filter-scope-selector.less +++ b/superset/assets/src/dashboard/stylesheets/filter-scope-selector.less @@ -19,7 +19,6 @@ @import "../../../stylesheets/less/cosmo/variables.less"; .filter-scope-container { - font-family: @font-family-sans-serif; font-size: 14px; .nav.nav-tabs { @@ -116,6 +115,7 @@ .rct-text { margin: 8px 0; + height: 30px; } } } @@ -135,7 +135,7 @@ padding: 8px 0; display: block; - &::before { + .type-indicator { border: 1px solid @gray-light; border-radius: 4px; padding: 2px 4px; @@ -146,9 +146,6 @@ &.chart { font-weight: normal; - &::before { - content: 'Chart'; - } } &.selected-filter { @@ -161,10 +158,6 @@ &.tab { font-weight: 700; - - &::before { - content: 'Tab'; - } } } @@ -213,7 +206,7 @@ font-weight: bold; } - // disable style from react-checkbox-tress.css + // disable style from react-checkbox-trees.css .rct-node-clickable:hover, .rct-node-clickable:focus, label:hover, @@ -234,7 +227,7 @@ } .filter-field-item { - padding: 0 50px; + padding: 0 16px 0 50px; margin-left: -50px; &.is-selected { diff --git a/superset/assets/src/dashboard/util/activeDashboardFilters.js b/superset/assets/src/dashboard/util/activeDashboardFilters.js index 3b20ea3..8fa00d0 100644 --- a/superset/assets/src/dashboard/util/activeDashboardFilters.js +++ b/superset/assets/src/dashboard/util/activeDashboardFilters.js @@ -17,7 +17,7 @@ * under the License. */ let activeFilters = {}; -let allFilterIds = []; +let allFilterBoxChartIds = []; export function getActiveFilters() { return activeFilters; @@ -28,17 +28,17 @@ export function getActiveFilters() { // after we make filterbox a dashboard build-in component, // will not need this check anymore export function isFilterBox(chartId) { - return allFilterIds.includes(chartId); + return allFilterBoxChartIds.includes(chartId); } -export function getAllFilterIds() { - return allFilterIds; +export function getAllFilterBoxChartIds() { + return allFilterBoxChartIds; } // non-empty filters from dashboardFilters, // this function does not take into account: filter immune or filter scope settings export function buildActiveFilters(allDashboardFilters = {}) { - allFilterIds = Object.values(allDashboardFilters).map( + allFilterBoxChartIds = Object.values(allDashboardFilters).map( filter => filter.chartId, ); diff --git a/superset/assets/src/dashboard/util/dashboardFiltersColorMap.js b/superset/assets/src/dashboard/util/dashboardFiltersColorMap.js index 55e0b72..129acf5 100644 --- a/superset/assets/src/dashboard/util/dashboardFiltersColorMap.js +++ b/superset/assets/src/dashboard/util/dashboardFiltersColorMap.js @@ -36,7 +36,7 @@ export function buildFilterColorMap(allDashboardFilters = {}) { Object.keys(columns) .sort() .forEach(column => { - const key = getDashboardFilterKey(chartId, column); + const key = getDashboardFilterKey({ chartId, column }); const colorCode = `badge-${filterColorIndex % FILTER_COLORS_COUNT}`; /* eslint-disable no-param-reassign */ colorMap[key] = colorCode; diff --git a/superset/assets/src/dashboard/util/getDashboardFilterKey.js b/superset/assets/src/dashboard/util/getDashboardFilterKey.js index aa65559..e6307c3 100644 --- a/superset/assets/src/dashboard/util/getDashboardFilterKey.js +++ b/superset/assets/src/dashboard/util/getDashboardFilterKey.js @@ -16,12 +16,12 @@ * specific language governing permissions and limitations * under the License. */ -export function getDashboardFilterKey(chartId, column) { +export function getDashboardFilterKey({ chartId, column }) { return `${chartId}_${column}`; } -export function getDashboardFilterByKey(key) { +export function getChartIdAndColumnFromFilterKey(key) { const [chartId, ...parts] = key.split('_'); - const columnName = parts.slice().join('_'); - return [parseInt(chartId, 10), columnName]; + const column = parts.slice().join('_'); + return { chartId: parseInt(chartId, 10), column }; } diff --git a/superset/assets/src/dashboard/util/getFilterFieldNodesTree.js b/superset/assets/src/dashboard/util/getFilterFieldNodesTree.js index bf74129..d82ea88 100644 --- a/superset/assets/src/dashboard/util/getFilterFieldNodesTree.js +++ b/superset/assets/src/dashboard/util/getFilterFieldNodesTree.js @@ -22,14 +22,10 @@ export default function getFilterFieldNodesTree({ dashboardFilters = {}, isSingleEditMode = true, }) { - if (Object.keys(dashboardFilters).length === 0) { - return []; - } - return Object.values(dashboardFilters).map(dashboardFilter => { const { chartId, filterName, columns, labels } = dashboardFilter; const children = Object.keys(columns).map(column => ({ - value: getDashboardFilterKey(chartId, column), + value: getDashboardFilterKey({ chartId, column }), label: labels[column] || column, showCheckbox: !isSingleEditMode, })); diff --git a/superset/assets/src/dashboard/util/getFilterScopeNodesTree.js b/superset/assets/src/dashboard/util/getFilterScopeNodesTree.js index d067f45..740ac0d 100644 --- a/superset/assets/src/dashboard/util/getFilterScopeNodesTree.js +++ b/superset/assets/src/dashboard/util/getFilterScopeNodesTree.js @@ -64,13 +64,13 @@ export default function getFilterScopeNodesTree({ let children = []; if (currentNode.children && currentNode.children.length) { currentNode.children.forEach(child => { - const cNode = traverse(components[child]); + const childNodeTree = traverse(components[child]); const childType = components[child].type; if (FILTER_SCOPE_CONTAINER_TYPES.includes(childType)) { - children.push(cNode); + children.push(childNodeTree); } else { - children = children.concat(cNode); + children = children.concat(childNodeTree); } }); } diff --git a/superset/assets/src/dashboard/util/getFilterScopeParentNodes.js b/superset/assets/src/dashboard/util/getFilterScopeParentNodes.js index 02a92a1..280661d 100644 --- a/superset/assets/src/dashboard/util/getFilterScopeParentNodes.js +++ b/superset/assets/src/dashboard/util/getFilterScopeParentNodes.js @@ -29,7 +29,7 @@ export default function getFilterScopeParentNodes(nodes, depthLimit = 0) { } }; - if (nodes && nodes.length) { + if (nodes && nodes.length > 0) { nodes.forEach(node => { traverse(node, 0); }); diff --git a/superset/assets/src/dashboard/util/propShapes.jsx b/superset/assets/src/dashboard/util/propShapes.jsx index 60b5f55..b59f41e 100644 --- a/superset/assets/src/dashboard/util/propShapes.jsx +++ b/superset/assets/src/dashboard/util/propShapes.jsx @@ -105,6 +105,30 @@ export const dashboardInfoPropShape = PropTypes.shape({ userId: PropTypes.string.isRequired, }); +/* eslint-disable-next-line no-undef */ +const lazyFunction = f => () => f().apply(this, arguments); + +const leafType = PropTypes.shape({ + value: PropTypes.oneOfType([PropTypes.number, PropTypes.string]).isRequired, + label: PropTypes.string.isRequired, +}); + +const parentShape = { + value: PropTypes.oneOfType([PropTypes.number, PropTypes.string]).isRequired, + label: PropTypes.string.isRequired, + children: PropTypes.arrayOf( + PropTypes.oneOfType([ + PropTypes.shape(lazyFunction(() => parentShape)), + leafType, + ]), + ), +}; + +export const filterScopeSelectorTreeNodePropShape = PropTypes.oneOfType([ + PropTypes.shape(parentShape), + leafType, +]); + export const loadStatsPropShape = PropTypes.objectOf( PropTypes.shape({ didLoad: PropTypes.bool.isRequired, diff --git a/superset/assets/src/visualizations/FilterBox/FilterBox.jsx b/superset/assets/src/visualizations/FilterBox/FilterBox.jsx index d4308f1..b259e7b 100644 --- a/superset/assets/src/visualizations/FilterBox/FilterBox.jsx +++ b/superset/assets/src/visualizations/FilterBox/FilterBox.jsx @@ -304,7 +304,7 @@ class FilterBox extends React.Component { } renderFilterBadge(chartId, column) { - const colorKey = getDashboardFilterKey(chartId, column); + const colorKey = getDashboardFilterKey({ chartId, column }); const filterColorMap = getFilterColorMap(); const colorCode = filterColorMap[colorKey];