From 0c0eccb81a95870c8be1f5ddfc1fc5926b889d4d Mon Sep 17 00:00:00 2001 From: "Michael S. Molina" <70410625+michael-s-molina@users.noreply.github.com> Date: Wed, 26 May 2021 04:49:00 -0300 Subject: [PATCH] chore: Improves the native filters UI/UX - iteration 3 (#14824) --- .../nativeFilters/NativeFiltersModal_spec.tsx | 2 +- .../FiltersConfigModal/FilterTabs.tsx | 4 +- .../FiltersConfigForm/CollapsibleControl.tsx | 70 +++++ .../FiltersConfigForm/ControlItems.tsx | 111 ------- .../FiltersConfigForm/FiltersConfigForm.tsx | 273 ++++++++++++------ ...s.test.tsx => getControlItemsMap.test.tsx} | 99 ++++--- .../FiltersConfigForm/getControlItemsMap.tsx | 110 +++++++ .../FiltersConfigModal/FiltersConfigModal.tsx | 2 +- 8 files changed, 440 insertions(+), 231 deletions(-) create mode 100644 superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/CollapsibleControl.tsx delete mode 100644 superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx rename superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/{ControlItems.test.tsx => getControlItemsMap.test.tsx} (62%) create mode 100644 superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/getControlItemsMap.tsx diff --git a/superset-frontend/spec/javascripts/dashboard/components/nativeFilters/NativeFiltersModal_spec.tsx b/superset-frontend/spec/javascripts/dashboard/components/nativeFilters/NativeFiltersModal_spec.tsx index 4b672b4a8..5c3551096 100644 --- a/superset-frontend/spec/javascripts/dashboard/components/nativeFilters/NativeFiltersModal_spec.tsx +++ b/superset-frontend/spec/javascripts/dashboard/components/nativeFilters/NativeFiltersModal_spec.tsx @@ -110,7 +110,7 @@ describe('FiltersConfigModal', () => { function addFilter() { act(() => { - wrapper.find('button[aria-label="Add tab"]').at(0).simulate('click'); + wrapper.find('[aria-label="Add filter"]').at(0).simulate('click'); }); } diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx index bab37da73..5020b5643 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterTabs.tsx @@ -167,7 +167,9 @@ const FilterTabs: FC = ({ addIcon={ {' '} - {t('Add filter')} + + {t('Add filter')} + } > diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/CollapsibleControl.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/CollapsibleControl.tsx new file mode 100644 index 000000000..1d9ed15aa --- /dev/null +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/CollapsibleControl.tsx @@ -0,0 +1,70 @@ +/** + * 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, { ReactNode, useState } from 'react'; +import { styled } from '@superset-ui/core'; +import { Checkbox } from 'src/common/components'; + +interface CollapsibleControlProps { + checked?: boolean; + title: string; + children: ReactNode; + onChange?: (checked: boolean) => void; +} + +const StyledContainer = styled.div<{ checked: boolean }>` + width: 100%; + display: flex; + flex-direction: column; + align-items: flex-start; + min-height: ${({ theme }) => theme.gridUnit * 10}px; + padding-top: ${({ theme }) => theme.gridUnit * 2 + 2}px; + + .checkbox { + margin-bottom: ${({ theme, checked }) => (checked ? theme.gridUnit : 0)}px; + } + + & > div { + margin-bottom: ${({ theme }) => theme.gridUnit * 2}px; + } +`; + +const CollapsibleControl = (props: CollapsibleControlProps) => { + const { checked = false, title, children, onChange } = props; + const [isChecked, setIsChecked] = useState(checked); + return ( + + { + const value = e.target.checked; + setIsChecked(value); + if (onChange) { + onChange(value); + } + }} + > + {title} + + {isChecked && children} + + ); +}; + +export { CollapsibleControl, CollapsibleControlProps }; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx deleted file mode 100644 index ae20a8886..000000000 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/ControlItems.tsx +++ /dev/null @@ -1,111 +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 { - CustomControlItem, - InfoTooltipWithTrigger, -} from '@superset-ui/chart-controls'; -import React, { FC } from 'react'; -import { Checkbox } from 'src/common/components'; -import { FormInstance } from 'antd/lib/form'; -import { getChartControlPanelRegistry, t } from '@superset-ui/core'; -import { Tooltip } from 'src/components/Tooltip'; -import { getControlItems, setNativeFilterFieldValues } from './utils'; -import { NativeFiltersForm, NativeFiltersFormItem } from '../types'; -import { StyledCheckboxFormItem } from './FiltersConfigForm'; -import { Filter } from '../../types'; - -type ControlItemsProps = { - disabled: boolean; - filterId: string; - forceUpdate: Function; - filterToEdit?: Filter; - form: FormInstance; - formFilter?: NativeFiltersFormItem; -}; - -const ControlItems: FC = ({ - disabled, - forceUpdate, - form, - filterId, - filterToEdit, - formFilter, -}) => { - const filterType = formFilter?.filterType; - - if (!filterType) return null; - - const controlPanelRegistry = getChartControlPanelRegistry(); - const controlItems = - getControlItems(controlPanelRegistry.get(filterType)) ?? []; - return ( - <> - {controlItems - .filter( - (controlItem: CustomControlItem) => - controlItem?.config?.renderTrigger, - ) - .map(controlItem => ( - - - { - if (!controlItem.config.resetConfig) { - forceUpdate(); - return; - } - setNativeFilterFieldValues(form, filterId, { - defaultDataMask: null, - }); - forceUpdate(); - }} - > - {controlItem.config.label}{' '} - {controlItem.config.description && ( - - )} - - - - ))} - - ); -}; -export default ControlItems; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx index 85e642c87..0966afb10 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -32,7 +32,7 @@ import { Metric, } from '@superset-ui/chart-controls'; import { FormInstance } from 'antd/lib/form'; -import React, { useCallback, useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useState, useMemo } from 'react'; import { useSelector } from 'react-redux'; import { Checkbox, Form, Input } from 'src/common/components'; import { Select } from 'src/components/Select'; @@ -45,7 +45,6 @@ import { addDangerToast } from 'src/messageToasts/actions'; import { ClientErrorObject } from 'src/utils/getClientErrorObject'; import SelectControl from 'src/explore/components/controls/SelectControl'; import Collapse from 'src/components/Collapse'; -import Button from 'src/components/Button'; import { getChartDataRequest } from 'src/chart/chartAction'; import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags'; import { waitForAsyncData } from 'src/middleware/asyncEvent'; @@ -60,10 +59,11 @@ import { import { useBackendFormUpdate } from './state'; import { getFormData } from '../../utils'; import { Filter } from '../../types'; -import ControlItems from './ControlItems'; +import getControlItemsMap from './getControlItemsMap'; import FilterScope from './FilterScope/FilterScope'; import RemovedFilter from './RemovedFilter'; import DefaultValue from './DefaultValue'; +import { CollapsibleControl } from './CollapsibleControl'; import { CASCADING_FILTERS, getFiltersConfigModalTestId, @@ -77,19 +77,29 @@ const StyledContainer = styled.div` justify-content: space-between; `; -const StyledDatasetContainer = styled.div` +const StyledRowContainer = styled.div` display: flex; flex-direction: row; justify-content: space-between; + width: 100%; `; export const StyledFormItem = styled(Form.Item)` width: 49%; margin-bottom: ${({ theme }) => theme.gridUnit * 4}px; + + & .ant-form-item-control-input { + min-height: ${({ theme }) => theme.gridUnit * 10}px; + } `; -export const StyledCheckboxFormItem = styled(Form.Item)` - margin-bottom: 0; +export const StyledRowFormItem = styled(Form.Item)` + margin-bottom: 0px; + min-width: 50%; + + & .ant-form-item-control-input { + min-height: ${({ theme }) => theme.gridUnit * 10}px; + } `; export const StyledLabel = styled.span` @@ -120,6 +130,10 @@ const StyledCollapse = styled(Collapse)` border: 0px; } + .ant-collapse-content-box { + padding-top: ${({ theme }) => theme.gridUnit * 2}px; + } + &.ant-collapse > .ant-collapse-item { border: 0px; border-radius: 0px; @@ -130,6 +144,10 @@ const StyledTabs = styled(Tabs)` .ant-tabs-nav-list { padding: 0px; } + + .ant-form-item-label { + padding-bottom: 0px; + } `; const FilterTabs = { @@ -169,8 +187,11 @@ const FILTERS_WITHOUT_COLUMN = [ 'filter_timecolumn', 'filter_groupby', ]; + const FILTERS_WITH_ADHOC_FILTERS = ['filter_select', 'filter_range']; +const BASIC_CONTROL_ITEMS = ['enableEmptyFilter', 'multiSelect']; + /** * The configuration form for a specific filter. * Assigns field values to `filters[filterId]` in the form. @@ -184,9 +205,14 @@ export const FiltersConfigForm: React.FC = ({ parentFilters, }) => { const [metrics, setMetrics] = useState([]); + const [hasDefaultValue, setHasDefaultValue] = useState( + !!filterToEdit?.defaultDataMask?.filterState?.value, + ); const forceUpdate = useForceUpdate(); const [datasetDetails, setDatasetDetails] = useState>(); - const formFilter = form.getFieldValue('filters')?.[filterId] || {}; + const defaultFormFilter = useMemo(() => {}, []); + const formFilter = + form.getFieldValue('filters')?.[filterId] || defaultFormFilter; const nativeFilterItems = getChartMetadataRegistry().items; const nativeFilterVizTypes = Object.entries(nativeFilterItems) // @ts-ignore @@ -243,7 +269,7 @@ export const FiltersConfigForm: React.FC = ({ useBackendFormUpdate(form, filterId); - const refreshHandler = () => { + const refreshHandler = useCallback(() => { if (!hasDataset || !formFilter?.dataset?.value) { forceUpdate(); return; @@ -287,7 +313,7 @@ export const FiltersConfigForm: React.FC = ({ forceUpdate(); } }); - }; + }, [filterId, forceUpdate, form, formFilter, hasDataset]); const defaultDatasetSelectOptions = Object.values(loadedDatasets).map( datasetToSelectOption, @@ -304,6 +330,19 @@ export const FiltersConfigForm: React.FC = ({ ...formFilter, }); + useEffect(() => { + if (hasDataset && hasFilledDataset && hasDefaultValue && isDataDirty) { + refreshHandler(); + } + }, [ + hasDataset, + hasFilledDataset, + hasDefaultValue, + formFilter, + isDataDirty, + refreshHandler, + ]); + const onDatasetSelectError = useCallback( ({ error, message }: ClientErrorObject) => { let errorText = message || error || t('An error has occurred'); @@ -324,8 +363,34 @@ export const FiltersConfigForm: React.FC = ({ label: filter.title, })); + const parentFilter = parentFilterOptions.find( + ({ value }) => value === filterToEdit?.cascadeParentIds[0], + ); + const showDefaultValue = !hasDataset || (!isDataDirty && hasFilledDataset); + const controlItems = formFilter + ? getControlItemsMap({ + disabled: !showDefaultValue, + forceUpdate, + form, + filterId, + filterType: formFilter.filterType, + filterToEdit, + }) + : {}; + + const onSortChanged = (value: boolean | undefined) => { + const previous = form.getFieldValue('filters')?.[filterId].controlValues; + setNativeFilterFieldValues(form, filterId, { + controlValues: { + ...previous, + sortAscending: value, + }, + }); + forceUpdate(); + }; + return ( <> @@ -367,7 +432,7 @@ export const FiltersConfigForm: React.FC = ({ {hasDataset && ( - + = ({ /> )} - + )} - + = ({ hidden initialValue={null} /> - {isCascadingFilter && ( - {t('Parent filter')}} - initialValue={parentFilterOptions.find( - ({ value }) => value === filterToEdit?.cascadeParentIds[0], - )} - data-test="parent-filter-input" - > - + + + )} + {Object.keys(controlItems) + .filter(key => !BASIC_CONTROL_ITEMS.includes(key)) + .map(key => controlItems[key])} {hasDataset && hasAdditionalFilters && ( - <> - + @@ -541,8 +608,8 @@ export const FiltersConfigForm: React.FC = ({ }} label={{t('Adhoc filters')}} /> - - + {t('Time range')}} initialValue={filterToEdit?.time_range || 'No filter'} @@ -556,37 +623,75 @@ export const FiltersConfigForm: React.FC = ({ forceUpdate(); }} /> - - + + )} - {hasMetrics && ( - {t('Sort Metric')}} - data-test="field-input" - > - ({ - value: metric.metric_name, - label: metric.verbose_name ?? metric.metric_name, - }))} - onChange={(value: string | null): void => { - if (value !== undefined) { - setNativeFilterFieldValues(form, filterId, { - sortMetric: value, - }); - forceUpdate(); + onSortChanged(checked || undefined)} + checked={ + typeof filterToEdit?.controlValues?.sortAscending === + 'boolean' + } + > + + {t('Sort type')}} + > +