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] Add loading indicator during debounce time #80158

Merged
merged 8 commits into from
Oct 29, 2020

Conversation

flash1293
Copy link
Contributor

This PR adds a loading indicator to the suggestions during the debounce time so they don't look stale

Old:
lagging_suggestions

New:
up_to_date_suggestions

@flash1293 flash1293 marked this pull request as ready for review October 12, 2020 15:16
@flash1293 flash1293 requested a review from a team October 12, 2020 15:16
@flash1293 flash1293 added Feature:Lens Team:Visualizations Visualization editors, elastic-charts and infrastructure labels Oct 12, 2020
@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.

The behavior does work as expected, but I have a suggestion for a different way of organizing the code.

@@ -135,10 +141,13 @@ const SuggestionPreview = ({
data-test-subj="lnsSuggestion"
onClick={onSelect}
>
{preview.expression && currentExpression !== renderedExpression && (
<EuiProgress size="xs" color="accent" position="absolute" />
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this approach works, but does it need to be Lens-specific? What if we supported a debouncing parameter on the ExpressionRenderer component which would handle this automatically? The main reason I'm suggesting this is that the ExpressionRenderer component is already responsible for showing both an EuiProgress and <EuiLoadingChart size="m" />, and it might be easy to share the logic.

Copy link
Contributor Author

@flash1293 flash1293 Oct 14, 2020

Choose a reason for hiding this comment

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

@elastic/kibana-app-arch what do you think about having a “debounce” parameter on the expression renderer component basically replicating this logic one layer deeper? The use case here is to not re-render the suggestions too often while the user interacts with the config, but still show an indication the current state will be updated soon.

Copy link
Member

Choose a reason for hiding this comment

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

i don't see a problem with introducing debounce parameter to expression renderer component

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Peter, I will move the logic over on this PR

@flash1293 flash1293 requested a review from a team as a code owner October 22, 2020 15:28
@flash1293
Copy link
Contributor Author

I moved the logic for debouncing into the react expression renderer component.

@botelastic botelastic bot added the Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) label Oct 22, 2020
@flash1293
Copy link
Contributor Author

Huh, I thought the react hooks package was already part of the bundle. I will look into a replacement

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.

Code LGTM, tested locally in Lens. I don't think there are any uses of this component outside Lens.

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.

Did not test since my last review, but I'm surprised about the react-use issue.

  1. Lens and other plugins are already importing react-use
  2. APM is importing it via import useDebounce from 'react-use/lib/useDebounce';
  3. Ingest manager has a custom definition of useDebounce separately from these

I'm fine with the choice to define an effect instead of increasing the import size, but I wonder if it would be simpler to accept the entire react-use package as a shared dependency.

@flash1293
Copy link
Contributor Author

@elasticmachine merge upstream

@flash1293 flash1293 requested a review from ppisljar October 29, 2020 08:28
@flash1293
Copy link
Contributor Author

flash1293 commented Oct 29, 2020

@wylieconlon I considered something along those lines, but it's not super straight forward, that's why I opted for the inline implementation:

  • By default, npm dependencies become part of each individual plugin bundle
  • The other plugins using it are doing so in async bundles, but this component is part of the initial bundle
  • We could move it to the shared dependencies, but AFAICT it would be the only place where it's used in the initial bundle, so it would still bloat the total initial JS download size compared to the current baseline
  • We could make this usage async as well by making the expressionrenderer component lazy, but that felt weird because it would mean creating a new async chunk with just that single component (which means another roundtrip until a Lens vis can be rendered)

For this use case I'm in favor of proceeding with the inline implementation as it's really straight forward and easy to read.

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

async chunks size

id before after diff
lens 1.0MB 1.0MB -101.0B

page load bundle size

id before after diff
expressions 179.3KB 180.0KB +704.0B

History

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

@flash1293 flash1293 requested a review from Dosant October 29, 2020 10:10
@Dosant
Copy link
Contributor

Dosant commented Oct 29, 2020

I'm fine with the choice to define an effect instead of increasing the import size, but I wonder if it would be simpler to accept the entire react-use package as a shared dependency.

@wylieconlon I've created an issue: #80181

Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

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

app-arch debounce code changes LGTM
didn't test
Issue for addressing react-use: #80181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Feature:Lens release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.11.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants