[Lens] Allows users to not render suggestions #115946

stratoula · 2021-10-21T13:57:10Z

Summary

This PR gives the users the ability to not render the suggestions.
The accordion status value is stored in localeStorage with the key LENS_SUGGESTIONS_PANEL_HIDDEN.

Note: When it is hidden, I don't send the search request so it could also be a performance improvement.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

stratoula · 2021-10-25T06:43:56Z

@MichaelMarcialis I want your feedback here!
First of all I used a switch but if you were thinking another component please let me know!
Secondly I wanted to give a transition to hiding and showing the panel. This is the reason that I am playing with max-height and the opacity (as on the display property we can't add transition and the result is not so smooth). How do you feel about it?

MichaelMarcialis · 2021-10-26T14:59:29Z

@MichaelMarcialis I want your feedback here!
First of all I used a switch but if you were thinking another component please let me know!
Secondly I wanted to give a transition to hiding and showing the panel. This is the reason that I am playing with max-height and the opacity (as on the display property we can't add transition and the result is not so smooth). How do you feel about it?

My first instinct would be to try using an EuiAccordion component here instead. That component already has show/hide transitions baked-in. Also, I believe that the accordion UI pattern makes more sense for such show/hide interactions.

stratoula · 2021-10-27T11:03:22Z

@elasticmachine merge upstream

stratoula · 2021-10-27T12:39:58Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.test.tsx

@@ -264,8 +264,10 @@ describe('suggestion_panel', () => {
      preloadedState,
    });

-    expect(instance.find(EuiIcon)).toHaveLength(1);
-    expect(instance.find(EuiIcon).prop('type')).toEqual(LensIconChartDatatable);
+    expect(instance.find('[data-test-subj="lnsSuggestionsPanel"]').find(EuiIcon)).toHaveLength(1);


I have to be more specific here as EuiAccordion introduces another EuiIcon component.

stratoula · 2021-10-27T13:03:51Z

@MichaelMarcialis great idea! It worked like a charm :)

elasticmachine · 2021-10-27T16:41:22Z

Pinging @elastic/kibana-vis-editors (Team:VisEditors)

dej611 · 2021-10-27T16:49:18Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

@@ -293,6 +296,15 @@ export function SuggestionPanel({
  }, [ExpressionRendererComponent]);

  const [lastSelectedSuggestion, setLastSelectedSuggestion] = useState<number>(-1);
+  // get user's selection from localStorage, this key defines if the suggestions panel will be hidden or not
+  const [hideSuggestions, setHideSuggestions] = useLocalStorage(


Haven't tested yet, maybe there's no difference, but can you prevent the suggestion computation above based on this flag?
I would be interesting to check if it has a performance impact

I had looked into it but it wasn't so simple. I could also get another look if you want.

I did some basic profiling with and without recomputing the suggestions even if hidden, and it does change much the performance. The expression evaluation is still the main bottleneck there, so strong opinion on this.

@dej611 the biggest problem I see is this:

if (suggestions.length === 0) { return null; }

When there are no suggestions then we don't display the suggestions panel. For example

if we don't compute them if the suggestions are hidden, how this will work? Can we use another variable? If yes, which?

MichaelMarcialis

This is looking good, @stratoula! Left a few comments/questions for your review.

MichaelMarcialis · 2021-10-28T13:34:34Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.scss

+.lnsSuggestionPanel--hideSuggestions {
+  padding-bottom: $euiSizeS;
+}


Rather than conditionally applying padding to the bottom of the suggestions panel, would it be better to just apply ever-present bottom padding to .lnsFrameLayout__pageBody? You'd likely also have to adjust or remove the padding from .lnsSuggestionPanel__suggestions as well, but I think that makes the organization and maintenance of the padding much simpler. What do you think?

@MichaelMarcialis the reason I didn't do it this way, is because I saw this comment and didn't want to break anything :D

.lnsFrameLayout__pageBody { @include euiScrollBar; min-width: $lnsPanelMinWidth + $euiSizeXL; overflow: hidden auto; display: flex; flex-direction: column; flex: 1 1 100%; // Leave out bottom padding so the suggestions scrollbar stays flush to window edge // Leave out left padding so the left sidebar's focus states are visible outside of content bounds // This also means needing to add same amount of margin to page content and suggestion items** padding: $euiSize $euiSize 0; position: relative; z-index: $lnsZLevel1; &:first-child { padding-left: $euiSize; } &.lnsFrameLayout__pageBody-isFullscreen { background: $euiColorEmptyShade; flex: 1; padding: 0; } }

But I removed the condition with this way:

removed the lnsSuggestionPanel--hideSuggestions

removed the padding from lnsSuggestionPanel__suggestions

added the padding on the container lnsSuggestionPanel
Wdyt about this?

That works. Thanks for looking into it!

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.scss

dej611

I did test locally with Chrome and Safari and all good 👍

Maybe the only minor feedback I can report is the use of the right arrow icon for the accordion feels weird, maybe it would look better as an arrow up? On click the content is rising up rather than pushing it down, which looks a bit different from classical accordion case.

Anyway approving to not block for that detail.

stratoula · 2021-10-29T13:29:55Z

@elasticmachine merge upstream

stratoula · 2021-11-01T07:02:23Z

@elasticmachine merge upstream

kibanamachine · 2021-11-01T15:05:48Z

💛 Build succeeded, but was flaky

Test Failures

[job] [logs] Default CI Group #6 / apis SecuritySolution Endpoints Matrix DNS Histogram Large data set Make sure that we get dns data without getting bucket errors when querying large volume of data

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`lens`	673	674	+1

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	953.0KB	953.9KB	+957.0B

History

💚 Build #3089 succeeded 0b510ef
💚 Build #2795 succeeded 48d0284
💚 Build #2061 succeeded f1216e2
💛 Build #1261 was flaky 1c42327
💚 Build #962 succeeded ffb096e

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

MichaelMarcialis

Thanks for addressing my comments. This LGTM!

* [Lens] Allows users to not render suggestions * Improve the transitions * change implementation and use EuiAccordion instead * Fixes the scrolling problem on the suggestions panel * Apply PR comments Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 29e807f) # Conflicts: # x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

* [Lens] Allows users to not render suggestions * Improve the transitions * change implementation and use EuiAccordion instead * Fixes the scrolling problem on the suggestions panel * Apply PR comments Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 29e807f)

flash1293 · 2022-01-14T09:13:31Z

💚 All backports created successfully

Status	Branch	Result
✅	8.0
✅	7.17

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

* [Lens] Allows users to not render suggestions * Improve the transitions * change implementation and use EuiAccordion instead * Fixes the scrolling problem on the suggestions panel * Apply PR comments Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 29e807f) # Conflicts: # x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx Co-authored-by: Stratoula Kalafateli <[email protected]>

* [Lens] Allows users to not render suggestions * Improve the transitions * change implementation and use EuiAccordion instead * Fixes the scrolling problem on the suggestions panel * Apply PR comments Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 29e807f) Co-authored-by: Stratoula Kalafateli <[email protected]>

stratoula added 2 commits October 21, 2021 16:56

[Lens] Allows users to not render suggestions

ffb096e

Improve the transitions

1c42327

kibanamachine and others added 2 commits October 27, 2021 07:03

Merge branch 'master' into lens-no-suggestions

e3f8f27

change implementation and use EuiAccordion instead

9084823

stratoula commented Oct 27, 2021

View reviewed changes

Fixes the scrolling problem on the suggestions panel

f1216e2

stratoula added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.1.0 release_note:enhancement labels Oct 27, 2021

stratoula marked this pull request as ready for review October 27, 2021 16:41

stratoula requested review from a team as code owners October 27, 2021 16:41

dej611 reviewed Oct 27, 2021

View reviewed changes

MichaelMarcialis reviewed Oct 28, 2021

View reviewed changes

dej611 approved these changes Oct 29, 2021

View reviewed changes

Merge branch 'master' into lens-no-suggestions

48d0284

kibanamachine and others added 2 commits November 1, 2021 03:02

Merge branch 'main' into lens-no-suggestions

0b510ef

Apply PR comments

12c5bf7

MichaelMarcialis approved these changes Nov 1, 2021

View reviewed changes

stratoula added the backport:skip This commit does not require backporting label Nov 2, 2021

stratoula merged commit 29e807f into elastic:main Nov 2, 2021

flash1293 added v7.17.0 v8.0.0 and removed backport:skip This commit does not require backporting labels Jan 14, 2022

flash1293 mentioned this pull request Jan 14, 2022

[8.0] [Lens] Allows users to not render suggestions (#115946) #123023

Merged

flash1293 mentioned this pull request Jan 14, 2022

[7.17] [Lens] Allows users to not render suggestions (#115946) #123024

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens] Allows users to not render suggestions #115946

[Lens] Allows users to not render suggestions #115946

stratoula commented Oct 21, 2021 •

edited

Loading

stratoula commented Oct 25, 2021

MichaelMarcialis commented Oct 26, 2021

stratoula commented Oct 27, 2021

stratoula Oct 27, 2021

stratoula commented Oct 27, 2021

elasticmachine commented Oct 27, 2021

dej611 Oct 27, 2021

stratoula Oct 29, 2021

dej611 Oct 29, 2021

stratoula Nov 1, 2021

MichaelMarcialis left a comment

MichaelMarcialis Oct 28, 2021

stratoula Nov 1, 2021 •

edited

Loading

MichaelMarcialis Nov 1, 2021

dej611 left a comment

stratoula commented Oct 29, 2021

stratoula commented Nov 1, 2021

kibanamachine commented Nov 1, 2021

MichaelMarcialis left a comment

flash1293 commented Jan 14, 2022

[Lens] Allows users to not render suggestions #115946

[Lens] Allows users to not render suggestions #115946

Conversation

stratoula commented Oct 21, 2021 • edited Loading

Summary

Checklist

stratoula commented Oct 25, 2021

MichaelMarcialis commented Oct 26, 2021

stratoula commented Oct 27, 2021

stratoula Oct 27, 2021

Choose a reason for hiding this comment

stratoula commented Oct 27, 2021

elasticmachine commented Oct 27, 2021

dej611 Oct 27, 2021

Choose a reason for hiding this comment

stratoula Oct 29, 2021

Choose a reason for hiding this comment

dej611 Oct 29, 2021

Choose a reason for hiding this comment

stratoula Nov 1, 2021

Choose a reason for hiding this comment

MichaelMarcialis left a comment

Choose a reason for hiding this comment

MichaelMarcialis Oct 28, 2021

Choose a reason for hiding this comment

stratoula Nov 1, 2021 • edited Loading

Choose a reason for hiding this comment

MichaelMarcialis Nov 1, 2021

Choose a reason for hiding this comment

dej611 left a comment

Choose a reason for hiding this comment

stratoula commented Oct 29, 2021

stratoula commented Nov 1, 2021

kibanamachine commented Nov 1, 2021

💛 Build succeeded, but was flaky

Test Failures

Metrics [docs]

Module Count

Async chunks

History

MichaelMarcialis left a comment

Choose a reason for hiding this comment

flash1293 commented Jan 14, 2022

💚 All backports created successfully

Questions ?

stratoula commented Oct 21, 2021 •

edited

Loading

stratoula Nov 1, 2021 •

edited

Loading