Skip to content
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

[Lens] refactor DimensionContainer and fix flyout bug #79277

Merged
merged 6 commits into from
Oct 5, 2020
Merged

[Lens] refactor DimensionContainer and fix flyout bug #79277

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1cc3a16
[Lens] fix pie chart bug on grouping order change and refactor Dimens…
mbondyra Oct 2, 2020
70eda9a
fix tests
mbondyra Oct 2, 2020
1786760
types
mbondyra Oct 2, 2020
21d284c
types
mbondyra Oct 2, 2020
4c43d3e
Merge branch 'master' into refactor_dimension_container
kibanamachine Oct 4, 2020
4248be1
Merge branch 'master' into refactor_dimension_container
kibanamachine Oct 5, 2020
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
9 changes: 0 additions & 9 deletions ...ugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,17 +13,8 @@
animation: euiFlyout $euiAnimSpeedNormal $euiAnimSlightResistance;
}

.lnsDimensionContainer--noAnimation {
animation: none;
}

.lnsDimensionContainer__footer,
.lnsDimensionContainer__header {
padding: $euiSizeS;
}

.lnsDimensionContainer__trigger {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this wasn't used anywhere

width: 100%;
display: block;
word-break: break-word;
}
96 changes: 20 additions & 76 deletions ...lugins/lens/public/editor_frame_service/editor_frame/config_panel/dimension_container.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/
import './dimension_container.scss';

import React, { useState, useEffect } from 'react';
import React, { useState } from 'react';
import {
EuiFlyoutHeader,
EuiFlyoutFooter,
Expand All @@ -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 ? (
<EuiFocusTrap disabled={!focusTrapIsEnabled} clickOutsideDisables={true}>
<EuiOutsideClickDetector onOutsideClick={closeFlyout} isDisabled={!flyoutIsVisible}>
<EuiOutsideClickDetector onOutsideClick={closeFlyout} isDisabled={!isOpen}>
<div
role="dialog"
aria-labelledby="lnsDimensionContainerTitle"
className={classNames('lnsDimensionContainer', {
'lnsDimensionContainer--noAnimation': openByCreation,
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly this line was responsible for directly showing the flyout without having it sliding out if it's a new dimension. Is there a special reason we don't want to do it anymore?

Copy link
Contributor Author

@mbondyra mbondyra Oct 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@flash1293 this is a weird construct that is a legacy from popover (each trigger has its own popover/flyout).

In master, when you open a new dimension, there is an animation of opening a flyout. Then, if you configure it to the correct dimension, under the hood we close 'empty-dimension' flyout and open the 'configured dimension' flyout (it has to be different because the trigger is different). But we want the user to perceive it as seamless change, without the animation of what's really happening (close new dimension flyout, open existing dimension flyout). Adding this classname was causing the animation not to trigger in this case so it looked like it's the same flyout.

In this PR, as we have one dimensionContainer for all of the triggers, when a user switches from new dimension to configured one, it's still the same DimensionContainer so nothing has to be hacked. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mbondyra Got it, thanks for the explanation!

})}
className="lnsDimensionContainer"
>
<EuiFlyoutHeader hasBorder className="lnsDimensionContainer__header">
<EuiTitle size="xs">
Expand All @@ -109,7 +53,14 @@ export function DimensionContainer({
iconType="sortLeft"
flush="left"
>
<strong>{panelTitle}</strong>
<strong>
{i18n.translate('xpack.lens.configure.configurePanelTitle', {
defaultMessage: '{groupLabel} configuration',
values: {
groupLabel,
},
})}
</strong>
</EuiButtonEmpty>
</EuiTitle>
</EuiFlyoutHeader>
Expand All @@ -126,12 +77,5 @@ export function DimensionContainer({
</div>
</EuiOutsideClickDetector>
</EuiFocusTrap>
);

return (
<>
{trigger}
{flyout}
</>
);
) : null;
}
23 changes: 10 additions & 13 deletions ...k/plugins/lens/public/editor_frame_service/editor_frame/config_panel/layer_panel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -211,7 +210,7 @@ describe('LayerPanel', () => {
groupId: 'a',
accessors: ['newid'],
filterOperations: () => true,
supportsMoreColumns: false,
supportsMoreColumns: true,
dataTestSubj: 'lnsGroup',
enableDimensionEditor: true,
},
Expand All @@ -220,11 +219,14 @@ describe('LayerPanel', () => {
mockVisualization.renderDimensionEditor = jest.fn();

const component = mountWithIntl(<LayerPanel {...getDefaultProps()} />);
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', () => {
Expand Down Expand Up @@ -263,11 +265,8 @@ describe('LayerPanel', () => {
});

const component = mountWithIntl(<LayerPanel {...getDefaultProps()} />);

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();

Expand Down Expand Up @@ -312,10 +311,8 @@ describe('LayerPanel', () => {

const component = mountWithIntl(<LayerPanel {...getDefaultProps()} />);

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);
Expand Down
Loading