[Lens] Use accordion menus in field list for available and empty fields #68871

mbondyra · 2020-06-11T08:53:28Z

Summary

Fixes #67203

check copy for warning messages with @cchaos

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser

For maintainers

This was checked for breaking API changes and was labeled appropriately

- Changed Field Filters component (once again) - Fixed some overflows of accordions

cchaos · 2020-06-11T21:05:35Z

@mbondyra This PR is fantastic! I didn't really have much design feedback, though I've been making more decisions on the Field filter button that I implemented. Here's a PR4U: mbondyra#1

Other than that I just have some lingering questions we probably need to answer.

1. Should we also align the list of fields in the config panel?

Meaning, use the same heading "Available fields" instead of "Individual fields" and should we include fields without data? Right now that list restricts to showing only the fields with dat.

2. Callout in the "Empty fields" section too?

When the empty fields section doesn't have any fields to show (usually because of some filters being applied) I think we need to show the callout in there as well.

3. Records item bug?

Not sure if this stems from this PR, but before I change any filters, the "Records" item is white indicating data. However, when I filter to only show the "Records" field type, it changes to look like it doesn't have any data.

@KOTungseth Here are a bunch of screenshots where text has been updated. Perhaps you can do a quick pass?

^^ That one, not shown, is that I've applied a global filter from the KQL bar

…rdion Design PR

mbondyra · 2020-06-12T13:12:53Z

Hi @cchaos thank you for all the feedback! In order with points:

I've merged your PR, thank you for that. I only have one concern with EuiSpacer removal. That causes the last element in accordion being cut. Also, when testing on macbook 13' screen, I looks like it'd be nice to add some margin in the bottom (I added <EuiSpacer size=l/> but let me know your thoughts!). Screenshots below:

Paddings inside accordion:

The black area is outside of the browser and I am already at the bottom of scroll:
3.

Done!
Yes, that's a good point, I'll add it in my next PR. Just one question: what happens if we have 50 available fields and 0 empty? Should we show callout for empty fields in this situation?
The bug was here before - it happens when the list of normal fields is empty (so we have no data) but it respects already filtered fields - I corrected it to respect fields before filtering. @wylieconlon can you confirm that the logic for special fields(so far records)?

cchaos · 2020-06-12T15:35:13Z

Wow good catch, I did not notice that all since it also seems really odd that it would behave that way. But found that adding padding to the accordion child wrapper directly was what was causing this. I think it will be slightly easier to manage with padding. Another PR coming your way.
I think we can use the same message as the Available fields

I also just saw you added a specific bullet for when there's text in the search box for the fields list. I think this is confusing since it sounded like it was talking about the global filters. I think you can use the same text as the "field filters" option

Thanks! Much better now
I think I'm walking back my idea to auto-expand all the sections when the user has added a filter or search. I think it could be annoying to the user to keep having to re-close any they don't care to see. I think the notification badge with the count should be enough to indicate that the lists have been filtered.

…rdion More spacing fixes

mbondyra · 2020-06-15T13:34:01Z

@elasticmachine merge upstream

mbondyra · 2020-06-18T11:27:18Z

@wylieconlon thanks so much for paying attention to this one! I've optimized some data transformation I put in wrong places and I've wrapped the FieldItem with debouncedComponent so it rerenders when user stops typing and not on every keystroke. In the end I managed to speed things up compared to master.

After:

Before (master):

wylieconlon

Overall LGTM- the performance is significantly improved compared to master, and the UX is better too. Good job!

wylieconlon · 2020-06-18T19:00:52Z

x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx

+ } else return 'emptyFields';
+ }),
+ };
+ }, [allFields, existingFields[currentIndexPattern.title], currentIndexPattern]);


I think that if we enable the linting rules for hooks, this would complain about missing an explicit dependency on hasSyncedExistingFields and defaultFieldGroups

wylieconlon · 2020-06-18T19:04:44Z

x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx

+ <EuiSpacer size="m" />
+ <EuiFlexGroup justifyContent="spaceAround">
+ <EuiFlexItem grow={false}>
+ <EuiLoadingSpinner size="m" />


I like the loading spinner, but we already had a loading indicator on the entire panel for this. This now creates two indicators for the same thing. I'm okay with it, but I do think @cchaos should opine.

I completely agree - that's why I named the commit 'controversial spinner' :D
I was trying to find a solution for the problem we currently have - glitch of NoFieldsCallout before existing fields are loaded, but I wanted to talk about it during sync. Video from the 7.8 cloud version, simulated 3g network below:
https://www.loom.com/share/3c398f5296be4cf3a89755041300e8d8

Instead of replacing the accordion content with the spinner, can you replace the notification count instead? This will better indicate that the number of fields is being updated as well.

wylieconlon · 2020-06-18T19:09:45Z

x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx

+ if (scrollContainer) {
+ const nearBottom =
+ scrollContainer.scrollTop + scrollContainer.clientHeight >
+ scrollContainer.scrollHeight * 0.9;


Scroll performance is significantly better in this update, but I would identify this function as an area of concern: it's reading DOM attributes like scrollTop which can be slow. It's possible that this function needs to be debounced, since scroll events on browsers like Chrome can happen within milliseconds of each other.

wylieconlon · 2020-06-18T19:14:13Z

x-pack/plugins/lens/public/indexpattern_datasource/datapanel.tsx

- </EuiCallOut>
- )}
+ )}
+ </EuiAccordion>


Both accordions seem to share a lot of common code: it might not be worth it, but can these be made into a function?

mbondyra · 2020-06-25T06:29:04Z

@elasticmachine merge upstream

mbondyra · 2020-06-25T12:22:09Z

@cchaos is this good enough?

@wylieconlon I addressed all your comments if you want to take a look again :)

wylieconlon

Changes LGTM!

cchaos

Looks great to me! Thank you @mbondyra! I know that was quite a few rounds of design input. But this is such a better experience overall.

I had just one last question with the loading icon, but approving whatever the answer is.

cchaos · 2020-06-26T13:45:57Z

x-pack/plugins/lens/public/indexpattern_datasource/fields_accordion.tsx

+ <EuiFlexGroup justifyContent="spaceAround">
+ <EuiFlexItem grow={false}>
+ <EuiLoadingSpinner size="m" />
+ </EuiFlexItem>
+ </EuiFlexGroup>


Is the flex group here necessary? Usually they're only useful when you have more than 1 item.

flash1293 · 2020-06-26T13:48:28Z

This looks almost perfect to me, thanks for the great work. I noticed one thing by setting the network latency to super high:

Spinner is not shown while the index pattern saved object itself is loaded (probably not relevant for real world use cases because this query is normally very fast)
While the field existence data is fetched, the "no data" callout is shown - seems like we want to hide that while the fetch is ongoing? (this is more noticable, especially when hitting more data)

Both aren't blockers for me to merge.

cchaos · 2020-06-26T13:54:09Z

++ To @flash1293's suggestions around loading.

mbondyra · 2020-06-26T14:51:56Z

While the field existence data is fetched, the "no data" callout is shown - seems like we want to hide that while the fetch is ongoing? (this is more noticable, especially when hitting more data)

Yep, I saw that too and tried to make it better - I replaced it with spinner that then I moved after design review to replace the number of fields. I thought we want the callout to stay 🙈 I am much happier with removing it when things are not loaded! 👍

Spinner is not shown while the index pattern saved object itself is loaded (probably not relevant for real world use cases because this query is normally very fast)

We don't use the smartest flag to indicate the data is loaded so I'd leave it outside of this PR.

kibanamachine · 2020-06-26T16:43:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d609e6a

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
lens	83	+5	78

History

💚 Build #56302 succeeded 65451f0
💚 Build #56266 succeeded 0ad4772
💚 Build #56182 succeeded bbca942
💚 Build #55057 succeeded 0063dbd
💔 Build #55014 failed 6112e7a

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

…ds (elastic#68871)

…ds (#68871) (#70095) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

* master: (59 commits) [Lens] Fix broken test (elastic#70117) [SIEM] Import timeline fix (elastic#65448) [SECURITY SOLUTION][INGEST] UX update for ingest manager edit/create datasource for endpoint (elastic#70079) [Telemetry] Collector Schema (elastic#64942) [Endpoint] Add Endpoint empty states for onboarding (elastic#69626) Hide unused resolver buttons (elastic#70112) [Security] `Investigate in Resolver` Timeline Integration (elastic#70111) [Discover] Improve styling of graphs in sidebar (elastic#69440) [Metrics UI] Fix EuiTheme type issue (elastic#69735) skip failing suite (elastic#70104) (elastic#70103) [ENDPOINT] Hide the Timeline Flyout while on the Management Pages (elastic#69998) [SIEM][CASE] Persist callout when dismissed (elastic#68372) [SIEM][Exceptions] - Cleaned up and updated exception list item comment structure (elastic#69532) [Maps] remove indexing state from redux (elastic#69765) Add API integration test for deleting data streams. (elastic#70020) renames SIEM to Security Solution (elastic#70070) Adding saved_objects_page in OSS (elastic#69900) [Lens] Use accordion menus in field list for available and empty fields (elastic#68871) Dynamic uiActions & license support (elastic#68507) [SIEM] Update readme for timeline apis (elastic#67038) ...

…bana into alerting/consumer-based-rbac * 'alerting/consumer-based-rbac' of github.com:gmmorris/kibana: (25 commits) [Lens] Fix broken test (elastic#70117) [SIEM] Import timeline fix (elastic#65448) [SECURITY SOLUTION][INGEST] UX update for ingest manager edit/create datasource for endpoint (elastic#70079) [Telemetry] Collector Schema (elastic#64942) [Endpoint] Add Endpoint empty states for onboarding (elastic#69626) Hide unused resolver buttons (elastic#70112) [Security] `Investigate in Resolver` Timeline Integration (elastic#70111) [Discover] Improve styling of graphs in sidebar (elastic#69440) [Metrics UI] Fix EuiTheme type issue (elastic#69735) skip failing suite (elastic#70104) (elastic#70103) [ENDPOINT] Hide the Timeline Flyout while on the Management Pages (elastic#69998) [SIEM][CASE] Persist callout when dismissed (elastic#68372) [SIEM][Exceptions] - Cleaned up and updated exception list item comment structure (elastic#69532) [Maps] remove indexing state from redux (elastic#69765) Add API integration test for deleting data streams. (elastic#70020) renames SIEM to Security Solution (elastic#70070) Adding saved_objects_page in OSS (elastic#69900) [Lens] Use accordion menus in field list for available and empty fields (elastic#68871) Dynamic uiActions & license support (elastic#68507) [SIEM] Update readme for timeline apis (elastic#67038) ...

mbondyra added 5 commits June 11, 2020 09:51

feat: accordion with state external

faee220

accordion with no external state

3b3af95

correct pagination

88290a4

test removing

da13172

fix i18n

d8e3cec

mbondyra force-pushed the lens/fielditems_empty_accordion branch from 369e077 to d8e3cec Compare June 11, 2020 12:32

Design PR

d13b2bc

- Changed Field Filters component (once again) - Fixed some overflows of accordions

mbondyra and others added 3 commits June 12, 2020 10:02

Merge pull request #1 from cchaos/mbondyra-lens/fielditems_empty_acco…

039d9b7

…rdion Design PR

fix: aligning the field-select

9bff7c8

refactor: group fields & add proper selecting of filtered groups

f4036c0

cchaos and others added 5 commits June 12, 2020 11:35

More spacing fixes

651d412

Merge pull request #2 from cchaos/mbondyra-lens/fielditems_empty_acco…

9442649

…rdion More spacing fixes

refactor: extract NoFieldsCallout

6b56c09

test added to no fields callout

4277633

tests updated

563f821

Merge branch 'master' into lens/fielditems_empty_accordion

f582e23

mbondyra marked this pull request as ready for review June 15, 2020 13:40

mbondyra requested a review from a team June 15, 2020 13:40

mbondyra requested a review from a team as a code owner June 15, 2020 13:40

mbondyra requested review from flash1293 and wylieconlon June 15, 2020 13:40

mbondyra added Feature:Lens release_note:enhancement Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.9.0 v8.0.0 labels Jun 15, 2020

mbondyra added 2 commits June 18, 2020 13:51

spinner tests

78d0582

feat: test of FieldItem without debounced hoc

6112e7a

elastic deleted a comment from kibanamachine Jun 18, 2020

types

0063dbd

wylieconlon approved these changes Jun 18, 2020

View reviewed changes

elasticmachine and others added 3 commits June 25, 2020 00:29

Merge branch 'master' into lens/fielditems_empty_accordion

bbca942

fix: spinner in right place, hooks rules

c2d8c16

perf: extracting fields_accordion & speeding up datapanel

0ad4772

tests added

65451f0

mbondyra force-pushed the lens/fielditems_empty_accordion branch from 5e69375 to 65451f0 Compare June 25, 2020 13:38

wylieconlon approved these changes Jun 25, 2020

View reviewed changes

mbondyra mentioned this pull request Jun 25, 2020

[Lens] Clean and inline disabling of react-hooks/exhaustive-deps eslint rule #70010

Merged

cchaos approved these changes Jun 26, 2020

View reviewed changes

don't show callout if not loaded

572d9a1

removing flexgroup

d609e6a

mbondyra merged commit 7440eea into elastic:master Jun 26, 2020

mbondyra deleted the lens/fielditems_empty_accordion branch June 26, 2020 16:43

mbondyra added a commit to mbondyra/kibana that referenced this pull request Jun 26, 2020

[Lens] Use accordion menus in field list for available and empty fiel…

1e1113a

…ds (elastic#68871)

mbondyra mentioned this pull request Jun 26, 2020

[7.x] [Lens] Use accordion menus in field list for available and empty fields (#68871) #70095

Merged

wylieconlon mentioned this pull request Jun 26, 2020

[Lens] Update screenshots in docs for 7.9 #70109

Closed

mbondyra added a commit that referenced this pull request Jun 27, 2020

[Lens] Use accordion menus in field list for available and empty fiel…

39102c3

…ds (#68871) (#70095) Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Use accordion menus in field list for available and empty fields #68871

[Lens] Use accordion menus in field list for available and empty fields #68871

mbondyra commented Jun 11, 2020 •

edited

Loading

cchaos commented Jun 11, 2020

mbondyra commented Jun 12, 2020 •

edited

Loading

cchaos commented Jun 12, 2020 •

edited

Loading

mbondyra commented Jun 15, 2020

mbondyra commented Jun 18, 2020 •

edited

Loading

wylieconlon left a comment

wylieconlon Jun 18, 2020

wylieconlon Jun 18, 2020

mbondyra Jun 18, 2020 •

edited

Loading

cchaos Jun 23, 2020

wylieconlon Jun 18, 2020

wylieconlon Jun 18, 2020

mbondyra commented Jun 25, 2020

mbondyra commented Jun 25, 2020

wylieconlon left a comment

cchaos left a comment

cchaos Jun 26, 2020

flash1293 commented Jun 26, 2020

cchaos commented Jun 26, 2020

mbondyra commented Jun 26, 2020

kibanamachine commented Jun 26, 2020

[Lens] Use accordion menus in field list for available and empty fields #68871

[Lens] Use accordion menus in field list for available and empty fields #68871

Conversation

mbondyra commented Jun 11, 2020 • edited Loading

Summary

Checklist

For maintainers

cchaos commented Jun 11, 2020

1. Should we also align the list of fields in the config panel?

2. Callout in the "Empty fields" section too?

3. Records item bug?

mbondyra commented Jun 12, 2020 • edited Loading

cchaos commented Jun 12, 2020 • edited Loading

mbondyra commented Jun 15, 2020

mbondyra commented Jun 18, 2020 • edited Loading

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Jun 18, 2020

Choose a reason for hiding this comment

wylieconlon Jun 18, 2020

Choose a reason for hiding this comment

mbondyra Jun 18, 2020 • edited Loading

Choose a reason for hiding this comment

cchaos Jun 23, 2020

Choose a reason for hiding this comment

wylieconlon Jun 18, 2020

Choose a reason for hiding this comment

wylieconlon Jun 18, 2020

Choose a reason for hiding this comment

mbondyra commented Jun 25, 2020

mbondyra commented Jun 25, 2020

wylieconlon left a comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

cchaos Jun 26, 2020

Choose a reason for hiding this comment

flash1293 commented Jun 26, 2020

cchaos commented Jun 26, 2020

mbondyra commented Jun 26, 2020

kibanamachine commented Jun 26, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

History

mbondyra commented Jun 11, 2020 •

edited

Loading

mbondyra commented Jun 12, 2020 •

edited

Loading

cchaos commented Jun 12, 2020 •

edited

Loading

mbondyra commented Jun 18, 2020 •

edited

Loading

mbondyra Jun 18, 2020 •

edited

Loading