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

[embeddable rebuild][controls] control group chaining #187877

Merged
merged 16 commits into from
Jul 16, 2024

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 9, 2024

PR implements chaining for refactored control group

@nreese
Copy link
Contributor Author

nreese commented Jul 9, 2024

/ci

@nreese
Copy link
Contributor Author

nreese commented Jul 9, 2024

/ci

@nreese nreese marked this pull request as ready for review July 9, 2024 21:25
@nreese nreese requested a review from a team as a code owner July 9, 2024 21:25
@nreese nreese added Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas release_note:skip Skip the PR/issue when compiling release notes project:embeddableRebuild v8.16.0 labels Jul 9, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-presentation (Team:Presentation)

@nreese
Copy link
Contributor Author

nreese commented Jul 9, 2024

/ci

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

@Heenawter Heenawter self-requested a review July 10, 2024 16:07
@nreese
Copy link
Contributor Author

nreese commented Jul 15, 2024

@elasticmachine merge upstream

Copy link
Contributor

@Heenawter Heenawter left a comment

Choose a reason for hiding this comment

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

This works great, and is way simpler to follow than the old system!! The only thing I noticed is that the time slider is outputting its filters without any sort of debounce, which is causing the range slider to flash into a loading state much quicker than it used to + there is a noticeable delay as I drag:

  • New controls:
    Jul-16-2024 08-35-00

  • Old controls:
    Jul-16-2024 08-37-07

In the old system, it looks like the time slider had a debounce of 500 on selections - not sure if it necessarily needs to be that high, but I think we should definitely debounce time slider selections :)

Comment on lines +40 to +46
const chainedControl$ = combineLatest([
apiPublishesFilters(chainedControlApi)
? chainedControlApi.filters$
: new BehaviorSubject(undefined),
apiPublishesTimeslice(chainedControlApi)
? chainedControlApi.timeslice$
: new BehaviorSubject(undefined),
Copy link
Contributor

Choose a reason for hiding this comment

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

So clean 🔥

@nreese
Copy link
Contributor Author

nreese commented Jul 16, 2024

The only thing I noticed is that the time slider is outputting its filters without any sort of debounce, which is causing the range slider to flash into a loading state much quicker than it used to + there is a noticeable delay as I drag

I will debounce timeslider in a separate PR since that is not related to this effort.

@nreese nreese merged commit 4043c7d into elastic:main Jul 16, 2024
21 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting project:embeddableRebuild release_note:skip Skip the PR/issue when compiling release notes Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas v8.16.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants