From 1cc3a16614a43bc26c33b32d4f664dfb498247dd Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Fri, 2 Oct 2020 14:15:16 +0200 Subject: [PATCH 1/4] [Lens] fix pie chart bug on grouping order change and refactor Dimension Container element --- .../config_panel/dimension_container.scss | 9 - .../config_panel/dimension_container.tsx | 96 ++----- .../editor_frame/config_panel/layer_panel.tsx | 253 ++++++++---------- .../editor_frame/config_panel/types.ts | 8 +- 4 files changed, 132 insertions(+), 234 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss index f200e25453a2a..54ceb77926931 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss @@ -13,17 +13,8 @@ animation: euiFlyout $euiAnimSpeedNormal $euiAnimSlightResistance; } -.lnsDimensionContainer--noAnimation { - animation: none; -} - .lnsDimensionContainer__footer, .lnsDimensionContainer__header { padding: $euiSizeS; } -.lnsDimensionContainer__trigger { - width: 100%; - display: block; - word-break: break-word; -} diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx index 19f4c0428260e..d53988adbe412 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx @@ -5,7 +5,7 @@ */ import './dimension_container.scss'; -import React, { useState, useEffect } from 'react'; +import React, { useState } from 'react'; import { EuiFlyoutHeader, EuiFlyoutFooter, @@ -16,89 +16,33 @@ import { EuiOutsideClickDetector, } from '@elastic/eui'; -import classNames from 'classnames'; import { i18n } from '@kbn/i18n'; -import { VisualizationDimensionGroupConfig } from '../../../types'; -import { DimensionContainerState } from './types'; export function DimensionContainer({ - dimensionContainerState, - setDimensionContainerState, - groups, - accessor, - groupId, - trigger, + isOpen, + groupLabel, + close, panel, - panelTitle, }: { - dimensionContainerState: DimensionContainerState; - setDimensionContainerState: (newState: DimensionContainerState) => void; - groups: VisualizationDimensionGroupConfig[]; - accessor: string; - groupId: string; - trigger: React.ReactElement; + isOpen: boolean; + close: () => void; panel: React.ReactElement; - panelTitle: React.ReactNode; + groupLabel: string; }) { - const [openByCreation, setIsOpenByCreation] = useState( - dimensionContainerState.openId === accessor - ); const [focusTrapIsEnabled, setFocusTrapIsEnabled] = useState(false); - const [flyoutIsVisible, setFlyoutIsVisible] = useState(false); - - const noMatch = dimensionContainerState.isOpen - ? !groups.some((d) => d.accessors.includes(accessor)) - : false; const closeFlyout = () => { - setDimensionContainerState({ - isOpen: false, - openId: null, - addingToGroupId: null, - }); - setIsOpenByCreation(false); + close(); setFocusTrapIsEnabled(false); - setFlyoutIsVisible(false); - }; - - const openFlyout = () => { - setFlyoutIsVisible(true); - setTimeout(() => { - setFocusTrapIsEnabled(true); - }, 255); }; - const flyoutShouldBeOpen = - dimensionContainerState.isOpen && - (dimensionContainerState.openId === accessor || - (noMatch && dimensionContainerState.addingToGroupId === groupId)); - - useEffect(() => { - if (flyoutShouldBeOpen) { - openFlyout(); - } - }); - - useEffect(() => { - if (!flyoutShouldBeOpen) { - if (flyoutIsVisible) { - setFlyoutIsVisible(false); - } - if (focusTrapIsEnabled) { - setFocusTrapIsEnabled(false); - } - } - }, [flyoutShouldBeOpen, flyoutIsVisible, focusTrapIsEnabled]); - - const flyout = flyoutIsVisible && ( + return isOpen ? ( - +
@@ -109,7 +53,14 @@ export function DimensionContainer({ iconType="sortLeft" flush="left" > - {panelTitle} + + {i18n.translate('xpack.lens.configure.configurePanelTitle', { + defaultMessage: '{groupLabel} configuration', + values: { + groupLabel, + }, + })} + @@ -126,12 +77,5 @@ export function DimensionContainer({
- ); - - return ( - <> - {trigger} - {flyout} - - ); + ) : null; } diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx index ce2955da890d7..c20b20029ad00 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx @@ -23,13 +23,12 @@ import { DragContext, DragDrop, ChildDragDropProvider } from '../../../drag_drop import { LayerSettings } from './layer_settings'; import { trackUiEvent } from '../../../lens_ui_telemetry'; import { generateId } from '../../../id_generator'; -import { ConfigPanelWrapperProps, DimensionContainerState } from './types'; +import { ConfigPanelWrapperProps, DimensionState } from './types'; import { DimensionContainer } from './dimension_container'; -const initialDimensionContainerState = { +const initialDimensionState = { isOpen: false, - openId: null, - addingToGroupId: null, + isNew: false, }; function isConfiguration( @@ -70,15 +69,13 @@ export function LayerPanel( } ) { const dragDropContext = useContext(DragContext); - const [dimensionContainerState, setDimensionContainerState] = useState( - initialDimensionContainerState - ); + const [dimensionState, setDimensionState] = useState(initialDimensionState); const { framePublicAPI, layerId, isOnlyLayer, onRemoveLayer, dataTestSubj } = props; const datasourcePublicAPI = framePublicAPI.datasourceLayers[layerId]; useEffect(() => { - setDimensionContainerState(initialDimensionContainerState); + setDimensionState(initialDimensionState); }, [props.activeVisualizationId]); if ( @@ -117,7 +114,7 @@ export function LayerPanel( const { groups } = activeVisualization.getConfiguration(layerVisualizationConfigProps); const isEmptyLayer = !groups.some((d) => d.accessors.length > 0); - + const { activeId, activeGroup } = dimensionState; return ( @@ -196,31 +193,6 @@ export function LayerPanel( > <> {group.accessors.map((accessor) => { - const datasourceDimensionEditor = ( - - ); - const visDimensionEditor = - activeVisualization.renderDimensionEditor && group.enableDimensionEditor ? ( -
- -
- ) : null; return (
- { - if (dimensionContainerState.isOpen) { - setDimensionContainerState(initialDimensionContainerState); - } else { - setDimensionContainerState({ - isOpen: true, - openId: accessor, - addingToGroupId: null, // not set for existing dimension - }); - } - }, - }} - /> - } - panel={ - <> - {datasourceDimensionEditor} - {visDimensionEditor} - - } - panelTitle={i18n.translate('xpack.lens.configure.configurePanelTitle', { - defaultMessage: '{groupLabel} configuration', - values: { - groupLabel: group.groupLabel, + { + if (dimensionState.isOpen) { + setDimensionState(initialDimensionState); + } else { + setDimensionState({ + isOpen: true, + isNew: false, + activeGroup: group, + activeId: accessor, + }); + } }, - })} + }} /> -
- { - if (dimensionContainerState.isOpen) { - setDimensionContainerState(initialDimensionContainerState); - } else { - setDimensionContainerState({ - isOpen: true, - openId: newId, - addingToGroupId: group.groupId, - }); - } - }} - > - - - } - panelTitle={i18n.translate('xpack.lens.configure.configurePanelTitle', { - defaultMessage: '{groupLabel} configuration', - values: { - groupLabel: group.groupLabel, - }, - })} - panel={ - { - props.updateAll( - datasourceId, - newState, - activeVisualization.setDimension({ - layerId, - groupId: group.groupId, - columnId: newId, - prevState: props.visualizationState, - }) - ); - setDimensionContainerState({ - isOpen: true, - openId: newId, - addingToGroupId: null, // clear now that dimension exists - }); - }, - }} - /> - } - /> + { + if (dimensionState.isOpen) { + setDimensionState(initialDimensionState); + } else { + setDimensionState({ + isOpen: true, + isNew: true, + activeGroup: group, + activeId: newId, + }); + } + }} + > + +
) : null} @@ -472,6 +379,60 @@ export function LayerPanel( ); })} + setDimensionState(initialDimensionState)} + panel={ + <> + {activeGroup && activeId && ( + { + props.updateAll( + datasourceId, + newState, + activeVisualization.setDimension({ + layerId, + groupId: activeGroup.groupId, + columnId: activeId, + prevState: props.visualizationState, + }) + ); + setDimensionState({ + ...dimensionState, + isNew: false, + }); + }, + }} + /> + )} + {activeGroup && + activeId && + !dimensionState.isNew && + activeVisualization.renderDimensionEditor && + activeGroup?.enableDimensionEditor && ( +
+ +
+ )} + + } + /> diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts index d42c5c3b99e53..de7d0049551bc 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts @@ -10,6 +10,7 @@ import { FramePublicAPI, Datasource, DatasourceDimensionEditorProps, + VisualizationDimensionGroupConfig, } from '../../../types'; export interface ConfigPanelWrapperProps { @@ -30,8 +31,9 @@ export interface ConfigPanelWrapperProps { core: DatasourceDimensionEditorProps['core']; } -export interface DimensionContainerState { +export interface DimensionState { isOpen: boolean; - openId: string | null; - addingToGroupId: string | null; + isNew: boolean; + activeId?: string; + activeGroup?: VisualizationDimensionGroupConfig; } From 70eda9a75c5ef5f1fa35109f082bac868a0f941f Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Fri, 2 Oct 2020 15:03:42 +0200 Subject: [PATCH 2/4] fix tests --- .../config_panel/layer_panel.test.tsx | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx index a9e2d6dc696ab..a3920abf54d1b 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx @@ -211,7 +211,7 @@ describe('LayerPanel', () => { groupId: 'a', accessors: ['newid'], filterOperations: () => true, - supportsMoreColumns: false, + supportsMoreColumns: true, dataTestSubj: 'lnsGroup', enableDimensionEditor: true, }, @@ -220,11 +220,14 @@ describe('LayerPanel', () => { mockVisualization.renderDimensionEditor = jest.fn(); const component = mountWithIntl(); + act(() => { + component.find('[data-test-subj="lns-empty-dimension"]').first().simulate('click'); + }); + component.update(); - const group = component.find('DimensionContainer'); - const panel = mount(group.prop('panel')); - - expect(panel.children()).toHaveLength(2); + const group = component.find('DimensionContainer').first(); + const panel: React.ReactElement = group.prop('panel'); + expect(panel.props.children).toHaveLength(2); }); it('should keep the DimensionContainer open when configuring a new dimension', () => { @@ -263,11 +266,8 @@ describe('LayerPanel', () => { }); const component = mountWithIntl(); - - const group = component.find('DimensionContainer'); - const triggerButton = mountWithIntl(group.prop('trigger')); act(() => { - triggerButton.find('[data-test-subj="lns-empty-dimension"]').first().simulate('click'); + component.find('[data-test-subj="lns-empty-dimension"]').first().simulate('click'); }); component.update(); @@ -312,10 +312,8 @@ describe('LayerPanel', () => { const component = mountWithIntl(); - const group = component.find('DimensionContainer'); - const triggerButton = mountWithIntl(group.prop('trigger')); act(() => { - triggerButton.find('[data-test-subj="lns-empty-dimension"]').first().simulate('click'); + component.find('[data-test-subj="lns-empty-dimension"]').first().simulate('click'); }); component.update(); expect(component.find('EuiFlyoutHeader').exists()).toBe(true); From 1786760a408e8131826faab3f1702e4167b4f314 Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Fri, 2 Oct 2020 15:25:12 +0200 Subject: [PATCH 3/4] types --- .../editor_frame/config_panel/layer_panel.test.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx index a3920abf54d1b..44dc22d20a4fe 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx @@ -14,7 +14,6 @@ import { } from '../../mocks'; import { ChildDragDropProvider } from '../../../drag_drop'; import { EuiFormRow } from '@elastic/eui'; -import { mount } from 'enzyme'; import { mountWithIntl } from 'test_utils/enzyme_helpers'; import { Visualization } from '../../../types'; import { LayerPanel } from './layer_panel'; From 21d284cb6ef0e80de923559eb5776219811e29cb Mon Sep 17 00:00:00 2001 From: Marta Bondyra Date: Fri, 2 Oct 2020 17:58:02 +0200 Subject: [PATCH 4/4] types --- .../config_panel/dimension_container.scss | 1 - .../config_panel/dimension_container.tsx | 17 ++++++-- .../editor_frame/config_panel/layer_panel.tsx | 39 +++++++++---------- .../editor_frame/config_panel/types.ts | 3 +- 4 files changed, 33 insertions(+), 27 deletions(-) diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss index 54ceb77926931..bd2789cf645c7 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss @@ -17,4 +17,3 @@ .lnsDimensionContainer__header { padding: $euiSizeS; } - diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx index d53988adbe412..8f1b441d1d285 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx @@ -5,7 +5,7 @@ */ import './dimension_container.scss'; -import React, { useState } from 'react'; +import React, { useState, useEffect } from 'react'; import { EuiFlyoutHeader, EuiFlyoutFooter, @@ -21,21 +21,30 @@ import { i18n } from '@kbn/i18n'; export function DimensionContainer({ isOpen, groupLabel, - close, + handleClose, panel, }: { isOpen: boolean; - close: () => void; + handleClose: () => void; panel: React.ReactElement; groupLabel: string; }) { const [focusTrapIsEnabled, setFocusTrapIsEnabled] = useState(false); const closeFlyout = () => { - close(); + handleClose(); setFocusTrapIsEnabled(false); }; + useEffect(() => { + if (isOpen) { + // without setTimeout here the flyout pushes content when animating + setTimeout(() => { + setFocusTrapIsEnabled(true); + }, 255); + } + }, [isOpen]); + return isOpen ? ( diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx index c20b20029ad00..e72bf75b010c3 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.tsx @@ -23,11 +23,10 @@ import { DragContext, DragDrop, ChildDragDropProvider } from '../../../drag_drop import { LayerSettings } from './layer_settings'; import { trackUiEvent } from '../../../lens_ui_telemetry'; import { generateId } from '../../../id_generator'; -import { ConfigPanelWrapperProps, DimensionState } from './types'; +import { ConfigPanelWrapperProps, ActiveDimensionState } from './types'; import { DimensionContainer } from './dimension_container'; -const initialDimensionState = { - isOpen: false, +const initialActiveDimensionState = { isNew: false, }; @@ -69,13 +68,15 @@ export function LayerPanel( } ) { const dragDropContext = useContext(DragContext); - const [dimensionState, setDimensionState] = useState(initialDimensionState); + const [activeDimension, setActiveDimension] = useState( + initialActiveDimensionState + ); const { framePublicAPI, layerId, isOnlyLayer, onRemoveLayer, dataTestSubj } = props; const datasourcePublicAPI = framePublicAPI.datasourceLayers[layerId]; useEffect(() => { - setDimensionState(initialDimensionState); + setActiveDimension(initialActiveDimensionState); }, [props.activeVisualizationId]); if ( @@ -114,7 +115,7 @@ export function LayerPanel( const { groups } = activeVisualization.getConfiguration(layerVisualizationConfigProps); const isEmptyLayer = !groups.some((d) => d.accessors.length > 0); - const { activeId, activeGroup } = dimensionState; + const { activeId, activeGroup } = activeDimension; return ( @@ -209,7 +210,7 @@ export function LayerPanel( : 'add' } data-test-subj={group.dataTestSubj} - draggable={!dimensionState.isOpen} + draggable={!activeId} value={{ columnId: accessor, groupId: group.groupId, layerId }} isValueEqual={isSameConfiguration} label={group.groupLabel} @@ -253,11 +254,10 @@ export function LayerPanel( filterOperations: group.filterOperations, suggestedPriority: group.suggestedPriority, onClick: () => { - if (dimensionState.isOpen) { - setDimensionState(initialDimensionState); + if (activeId) { + setActiveDimension(initialActiveDimensionState); } else { - setDimensionState({ - isOpen: true, + setActiveDimension({ isNew: false, activeGroup: group, activeId: accessor, @@ -355,11 +355,10 @@ export function LayerPanel( }} data-test-subj="lns-empty-dimension" onClick={() => { - if (dimensionState.isOpen) { - setDimensionState(initialDimensionState); + if (activeId) { + setActiveDimension(initialActiveDimensionState); } else { - setDimensionState({ - isOpen: true, + setActiveDimension({ isNew: true, activeGroup: group, activeId: newId, @@ -380,9 +379,9 @@ export function LayerPanel( ); })} setDimensionState(initialDimensionState)} + handleClose={() => setActiveDimension(initialActiveDimensionState)} panel={ <> {activeGroup && activeId && ( @@ -405,8 +404,8 @@ export function LayerPanel( prevState: props.visualizationState, }) ); - setDimensionState({ - ...dimensionState, + setActiveDimension({ + ...activeDimension, isNew: false, }); }, @@ -415,7 +414,7 @@ export function LayerPanel( )} {activeGroup && activeId && - !dimensionState.isNew && + !activeDimension.isNew && activeVisualization.renderDimensionEditor && activeGroup?.enableDimensionEditor && (
diff --git a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts index de7d0049551bc..c172c6da6848c 100644 --- a/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts +++ b/x-pack/plugins/lens/public/editor_frame_service/editor_frame/config_panel/types.ts @@ -31,8 +31,7 @@ export interface ConfigPanelWrapperProps { core: DatasourceDimensionEditorProps['core']; } -export interface DimensionState { - isOpen: boolean; +export interface ActiveDimensionState { isNew: boolean; activeId?: string; activeGroup?: VisualizationDimensionGroupConfig;