[Lens] Move configuration popover to flyout #76046

cchaos · 2020-08-26T22:09:49Z

Fixes #75568

This PR changes the Lens configuration popover to be a flyout (contained within the sidebar). It doesn't use EuiFlyout because we want it to be specially positioned. But it does use the EUI flyout mixin to align styling.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This was checked for cross-browser compatibility

cchaos · 2020-08-26T22:36:23Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/field_select.tsx

-      isInvalid={Boolean(incompatibleSelectedOperationType && selectedColumnOperationType)}
+      isInvalid={Boolean(incompatibleSelectedOperationType)}


I removed the info style callout about the field selection missing in favor of now just showing it as invalid if they've already chosen an aggregation. It keeps the content from jumping around with the callout jumping in and out.

But what I do need help with is that once the user selects a field, it completely re-renders the flyout, therefore restarting the fly in animation.

cchaos · 2020-08-26T22:57:34Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/popover_editor.tsx

+        <EuiFormLabel>Choose a function</EuiFormLabel>
+        <EuiSpacer size="s" />
+        <EuiListGroup
+          className={sideNavItems.length > 3 ? 'lnsIndexPatternDimensionEditor__columns' : ''}
+          gutterSize="none"
+          listItems={sideNavItems}
+          maxWidth={false}
+        />


I need to check the a11y of this group. Likely needs to change to some sort of fieldset and legend combo.

cchaos · 2020-08-26T23:07:56Z

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/popover_editor.tsx

-                  <EuiCallOut
-                    data-test-subj="indexPattern-invalid-operation"
-                    title={i18n.translate('xpack.lens.indexPattern.invalidOperationLabel', {
-                      defaultMessage: 'To use this function, select a different field.',
-                    })}
-                    color="warning"
-                    size="s"
-                    iconType="sortUp"
-                  />


This callout is also gone in favor of helpText on the input itself. Which means the DTS's for both callouts are gone and I'll need help updating the tests.

cchaos · 2020-08-26T23:24:32Z

@wylieconlon / @mbondyra : I'll need help fixing the tests on this one. I still have to evaluate a bit of accessibility and browser test, but those should be minimal and shouldn't affect tests. Please feel free to commit directly to this branch.

dej611 · 2020-09-02T14:25:31Z

I can help with the tests @cchaos

mbondyra · 2020-09-09T13:15:14Z

@cchaos I've submitted a PR with naming changes. Other than that, is this PR still in the draft mode? I'm happy to merge it as it is (after naming changes), I just have one question: is it intentional that the copy for flyout title for non configured dimension is not the same as for the configured one (x-axis vs x-axis configuration)? If so, why is this?

mbondyra

Tested on Firefox and Chrome.
Code looks good to me 🥇
Thanks for a great job!

cchaos · 2020-09-09T14:14:02Z

Thanks @mbondyra and @dej611 for taking this one forward!

I've submitted a PR with naming changes.

I commented on that PR about the name itself. But I can pull this one out of draft now and see if there's time to merge that one first or do a follow up.

is it intentional that the copy for flyout title for non configured dimension is not the same as for the configured one (x-axis vs x-axis configuration)? If so, why is this?

This was not intentional but an oversight. If you know where that title is can you update it by adding configuration?

mbondyra · 2020-09-09T14:21:33Z

@elasticmachine merge upstream

cchaos · 2020-09-09T21:25:39Z

Hey @mbondyra I found some a11y issues that needed fixing, but in turn screwed up the it('should close the DimensionContainer when the active visualization changes') test. Could you have a look at the new useEffect method I'm using in the DimensionContainer?

wylieconlon

Looking and feeling good! Found a bug, steps to reproduce:

Start from an already-configured XY chart
Click the empty Y axis that isn't configured yet
Once you finish configuring the minimum required fields (just Count, or field + function) the flyout stays in the empty state
When you close the flyout, it reveals that there were actually 2 flyouts: the empty one, and the previously configured dimension

In general, I found that I was really missing the ability to click outside the flyout to close it, like we previously had with the popover. It feels like this is higher-commitment flow because it's harder to close. Maybe I'll get used to it, but if we can bring it back I think we should.

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx

flash1293 · 2020-09-10T09:17:14Z

I pushed a fix for the hidden popover - this should also fix the tests.

cchaos · 2020-09-10T17:10:54Z

When you close the flyout, it reveals that there were actually 2 flyouts: the empty one, and the previously configured dimension

I'm not really sure how to solve this one as I think it has to do with something outside of the flyout itself.

In general, I found that I was really missing the ability to click outside the flyout to close it, like we previously had with the popover. It feels like this is higher-commitment flow because it's harder to close. Maybe I'll get used to it, but if we can bring it back I think we should.

I totally understand and agree that the extra click can be a bit annoying. However, I do see it as a feature that it stays open as you play with other settings and we don't want to obscure the chart with the mask overlay. One thing I thought about doing is somehow adding a timer that starts when focus leaves the flyout and closes the flyout automatically after like 5 seconds or something.

flash1293 · 2020-09-10T17:25:52Z

@cchaos the first part with the empty flyout is what I fixed earlier today

flash1293

Reviewed and looks mostly good to me. The readme (https://github.com/elastic/kibana/blob/9186171ad160ead3fb9ec977f3465904fd293aa0/x-pack/plugins/lens/readme.md) is still referencing the popover, could you update it?

flash1293 · 2020-09-14T09:18:32Z

@elasticmachine merge upstream

kibanamachine · 2020-09-14T21:30:54Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 82743f0

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
lens	433	+18	415

page load bundle size

id	value	diff	baseline
lens	1.0MB	+14.6KB	1.0MB

History

💚 Build #74053 succeeded 25b6412
💚 Build #73896 succeeded 8c61729
💚 Build #73726 succeeded 3763a3316cf7ba0b782b6b5ca0b89c9a6bd30315
💚 Build #73621 succeeded 09e4f9cf75566e0c6b7c8127b6f6b911792bb3ef
💔 Build #73542 failed ac463d128e1e6ebd300fabaecf16c487c9f577f2

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

wylieconlon

LGTM, tested and couldn't find any issues!

* Moving to a Flyout implementation * Fix up inner layout of flyout * Fix up form rows

* master: (25 commits) [Security Solution] Add unit tests for Network search strategy (elastic#77416) [Alerting] Improves performance of the authorization filter in AlertsClient.find by skipping KQL parsing (elastic#77040) [Ingest Manager] Add route for package installation by upload (elastic#77044) [APM-UI][E2E] filter PRs from the uptime GH team (elastic#77359) [APM] Remove useLocation and some minor route improvements (elastic#76343) [Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper (elastic#77258) [SECURITY_SOLUTION] Task/hostname policy response ux updates (elastic#76444) Move remaining uses of serviceName away from urlParams (elastic#77248) [Lens] Move configuration popover to flyout (elastic#76046) [Ingest Manager] Manually build Fleet kuery with Node arguments (elastic#76589) skip flaky suite (elastic#59975) Neutral-naming in reporting plugin (elastic#77371) [Enterprise Search] Add UserIcon styles (elastic#77385) [RUM Dashboard] Added loading state to visitor breakdown pie charts (elastic#77201) [Ingest Manager] Fix polling for new agent action (elastic#77339) Remote cluster - Functional UI test to change the superuser to a test_user with limited role (elastic#77212) Stacked headers and navigational search (elastic#72331) [ML] DF Analytics creation wizard: Fixing field loading race condition (elastic#77326) [Monitoring] Handle no mappings found for sort and collapse fields (elastic#77099) Add Lens to Recently Accessed (elastic#77249) ...

* master: (293 commits) Fix tsvb filter ration for table (elastic#77272) [Security Solution] Add unit tests for Network search strategy (elastic#77416) [Alerting] Improves performance of the authorization filter in AlertsClient.find by skipping KQL parsing (elastic#77040) [Ingest Manager] Add route for package installation by upload (elastic#77044) [APM-UI][E2E] filter PRs from the uptime GH team (elastic#77359) [APM] Remove useLocation and some minor route improvements (elastic#76343) [Enterprise Search] Update enterpriseSearchRequestHandler to manage range of errors + add handleAPIErrors helper (elastic#77258) [SECURITY_SOLUTION] Task/hostname policy response ux updates (elastic#76444) Move remaining uses of serviceName away from urlParams (elastic#77248) [Lens] Move configuration popover to flyout (elastic#76046) [Ingest Manager] Manually build Fleet kuery with Node arguments (elastic#76589) skip flaky suite (elastic#59975) Neutral-naming in reporting plugin (elastic#77371) [Enterprise Search] Add UserIcon styles (elastic#77385) [RUM Dashboard] Added loading state to visitor breakdown pie charts (elastic#77201) [Ingest Manager] Fix polling for new agent action (elastic#77339) Remote cluster - Functional UI test to change the superuser to a test_user with limited role (elastic#77212) Stacked headers and navigational search (elastic#72331) [ML] DF Analytics creation wizard: Fixing field loading race condition (elastic#77326) [Monitoring] Handle no mappings found for sort and collapse fields (elastic#77099) ...

* Moving to a Flyout implementation * Fix up inner layout of flyout * Fix up form rows

cchaos commented Aug 26, 2020

View reviewed changes

dej611 added the Feature:Lens label Sep 2, 2020

mbondyra approved these changes Sep 9, 2020

View reviewed changes

cchaos added release_note:skip Skip the PR/issue when compiling release notes v7.10.0 v8.0.0 labels Sep 9, 2020

cchaos marked this pull request as ready for review September 9, 2020 14:15

cchaos requested a review from a team September 9, 2020 14:15

cchaos requested a review from a team as a code owner September 9, 2020 14:15

wylieconlon reviewed Sep 9, 2020

View reviewed changes

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx Outdated Show resolved Hide resolved

x-pack/plugins/lens/public/indexpattern_datasource/dimension_panel/dimension_editor.tsx Show resolved Hide resolved

cchaos requested a review from wylieconlon September 10, 2020 18:16

cchaos and others added 10 commits September 10, 2020 16:59

Moving to a Flyout implementation

c33d67f

Fix up inner layout of flyout

2bb56c5

Fix up form rows

d4da924

A few more adjustments for flyout

e5fe6d8

Cleanup

62a2237

Fix 2 tests

ef1cbde

i18n

106e868

✅ Fix Jest tests

d943cb4

🏷️ Fix unused import

c7fca8e

fix functional tests

c4bcaa4

mbondyra and others added 12 commits September 10, 2020 16:59

bad revert

21f1eea

revert to not inlining

721942d

naming from popover to flyout

54f57b0

fix: double flyout

e541bb4

fix: don't allow to drag when flyout is open

1d69b5a

the same titles for configured/non-configured flyouts

d074a8f

refactor names

a390d35

controversial state naming changed

e2e128b

Fixing keyboard navigation/a11y

b6d416e

fix double flyout

fb38c81

i18n

90e1dda

Fix other conflicts

8c61729

cchaos force-pushed the lens/config_flyout branch from 3763a33 to 8c61729 Compare September 10, 2020 22:51

flash1293 reviewed Sep 14, 2020

View reviewed changes

elasticmachine and others added 2 commits September 14, 2020 03:18

Merge branch 'master' into lens/config_flyout

25b6412

Update readme

82743f0

cchaos requested a review from flash1293 September 14, 2020 19:56

wylieconlon approved these changes Sep 14, 2020

View reviewed changes

cchaos merged commit 5a2f7ae into elastic:master Sep 14, 2020

cchaos mentioned this pull request Sep 14, 2020

[7.x] [Lens] Move configuration popover to flyout (#76046) #77417

Closed

cchaos added a commit to cchaos/kibana that referenced this pull request Sep 14, 2020

[Lens] Move configuration popover to flyout (elastic#76046)

228c7a1

* Moving to a Flyout implementation * Fix up inner layout of flyout * Fix up form rows

cchaos deleted the lens/config_flyout branch September 14, 2020 22:29

cchaos mentioned this pull request Sep 15, 2020

[7.x] [Lens] Move configuration popover to flyout (#76046) #77487

Merged

cchaos added a commit to cchaos/kibana that referenced this pull request Sep 15, 2020

[Lens] Move configuration popover to flyout (elastic#76046)

87f04f3

* Moving to a Flyout implementation * Fix up inner layout of flyout * Fix up form rows

cchaos added a commit that referenced this pull request Sep 15, 2020

[Lens] Move configuration popover to flyout (#76046) (#77487)

2d4cdcf

* Moving to a Flyout implementation * Fix up inner layout of flyout * Fix up form rows

cchaos mentioned this pull request Sep 22, 2020

[Lens] Make it easier to exit the configuration flyout #78195

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Move configuration popover to flyout #76046

[Lens] Move configuration popover to flyout #76046

cchaos commented Aug 26, 2020 •

edited

Loading

cchaos Aug 26, 2020

cchaos Aug 26, 2020

mbondyra Sep 8, 2020

cchaos Aug 26, 2020

cchaos Aug 26, 2020

cchaos commented Aug 26, 2020

dej611 commented Sep 2, 2020

mbondyra commented Sep 9, 2020 •

edited

Loading

mbondyra left a comment

cchaos commented Sep 9, 2020

mbondyra commented Sep 9, 2020

cchaos commented Sep 9, 2020

wylieconlon left a comment

flash1293 commented Sep 10, 2020

cchaos commented Sep 10, 2020

flash1293 commented Sep 10, 2020

flash1293 left a comment

flash1293 commented Sep 14, 2020

kibanamachine commented Sep 14, 2020

wylieconlon left a comment

		isInvalid={Boolean(incompatibleSelectedOperationType && selectedColumnOperationType)}
		isInvalid={Boolean(incompatibleSelectedOperationType)}

[Lens] Move configuration popover to flyout #76046

[Lens] Move configuration popover to flyout #76046

Conversation

cchaos commented Aug 26, 2020 • edited Loading

Fixes #75568

Checklist

cchaos Aug 26, 2020

Choose a reason for hiding this comment

cchaos Aug 26, 2020

Choose a reason for hiding this comment

mbondyra Sep 8, 2020

Choose a reason for hiding this comment

cchaos Aug 26, 2020

Choose a reason for hiding this comment

cchaos Aug 26, 2020

Choose a reason for hiding this comment

cchaos commented Aug 26, 2020

dej611 commented Sep 2, 2020

mbondyra commented Sep 9, 2020 • edited Loading

mbondyra left a comment

Choose a reason for hiding this comment

cchaos commented Sep 9, 2020

mbondyra commented Sep 9, 2020

cchaos commented Sep 9, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

flash1293 commented Sep 10, 2020

cchaos commented Sep 10, 2020

flash1293 commented Sep 10, 2020

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 commented Sep 14, 2020

kibanamachine commented Sep 14, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

page load bundle size

History

wylieconlon left a comment

Choose a reason for hiding this comment

cchaos commented Aug 26, 2020 •

edited

Loading

mbondyra commented Sep 9, 2020 •

edited

Loading