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.

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] Avoid suggestion rendering and evaluation on fullscreen mode #102757

Merged
merged 3 commits into from
Jun 23, 2021

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Jun 21, 2021

Summary

Avoid to compute suggestions when the Formula editor is in fullscreen mode: expecially when editing a formula this should lead to better performance.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dej611 dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.14.0 auto-backport Deprecated - use backport:version if exact versions are needed labels Jun 21, 2021
@dej611 dej611 requested a review from a team June 21, 2021 15:06
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app (Team:KibanaApp)

Copy link
Contributor

@wylieconlon wylieconlon left a comment

Choose a reason for hiding this comment

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

@dej611 LGTM, although we might not need this line of CSS any more: .lnsFrameLayout-isFullscreen .lnsFrameLayout__suggestionPanel. There will definitely be a rerender and refetch when we show suggestions again, but it should improve the performance while viewing just the main chart.

@dej611 dej611 requested a review from a team as a code owner June 22, 2021 08:35
@dej611
Copy link
Contributor Author

dej611 commented Jun 22, 2021

@dej611 LGTM, although we might not need this line of CSS any more: .lnsFrameLayout-isFullscreen .lnsFrameLayout__suggestionPanel. There will definitely be a rerender and refetch when we show suggestions again, but it should improve the performance while viewing just the main chart.

Yes, the fix aims at improve the performance in a specific context, the formula fullscreen, slowing down a bit the collapse transition. Ideally it's multiple speedups (formula editing) vs one slowdown (editor collapse).

Copy link
Contributor

@MichaelMarcialis MichaelMarcialis left a comment

Choose a reason for hiding this comment

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

Left one small nit about a CSS comment, but LGTM otherwise. Approving.

@@ -88,8 +88,7 @@ a tilemap in an iframe: https://github.com/elastic/kibana/issues/16457 */
position: relative;
}

.lnsFrameLayout-isFullscreen .lnsFrameLayout__sidebar--left,
.lnsFrameLayout-isFullscreen .lnsFrameLayout__suggestionPanel {
.lnsFrameLayout-isFullscreen .lnsFrameLayout__sidebar--left {
// Hide the datapanel and suggestions in fullscreen mode. Using display: none does trigger
Copy link
Contributor

Choose a reason for hiding this comment

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

Small nit, but might be worth updating this comment to no longer reference suggestions.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
lens 1.5MB 1.5MB -217.0B

History

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

@dej611 dej611 merged commit 7733950 into elastic:master Jun 23, 2021
@dej611 dej611 deleted the fix/avoid-suggestions-on-fullscreen branch June 23, 2021 08:27
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Jun 23, 2021
…102757) (#103054)

Co-authored-by: Marco Liberati <dej611@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants