Skip to content

[Lens] Debounce axis name inputs mob programming #100108

Merged
mbondyra merged 9 commits intoelastic:masterfrom
mbondyra:lens/debounce_axis_name_inputs
May 18, 2021
Merged

[Lens] Debounce axis name inputs mob programming #100108
mbondyra merged 9 commits intoelastic:masterfrom
mbondyra:lens/debounce_axis_name_inputs

Conversation

@mbondyra
Copy link
Copy Markdown
Contributor

@mbondyra mbondyra commented May 14, 2021

Summary

Fixes #98719

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@mbondyra mbondyra added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// 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 May 14, 2021
@mbondyra mbondyra force-pushed the lens/debounce_axis_name_inputs branch from 36db12c to 4b5aef8 Compare May 14, 2021 11:30
@mbondyra mbondyra force-pushed the lens/debounce_axis_name_inputs branch from 08161fa to 1733711 Compare May 14, 2021 13:23
@mbondyra mbondyra requested review from dej611, flash1293 and majagrubic and removed request for flash1293 May 14, 2021 13:24
@mbondyra mbondyra force-pushed the lens/debounce_axis_name_inputs branch from c125a9e to c7de6d2 Compare May 14, 2021 13:31
@mbondyra mbondyra marked this pull request as ready for review May 14, 2021 13:33
@mbondyra mbondyra requested a review from a team May 14, 2021 13:33
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented May 14, 2021

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

mbondyra
mbondyra commented May 14, 2021
View reviewed changes
@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented May 14, 2021

I'm looking into this failure. Seems like this is solution is somehow incompatible with using jest fake timers.

@dej611
Copy link
Copy Markdown
Contributor

dej611 commented May 14, 2021

In the ranges tests I was manually directing the jest fake times. Maybe we can build some utility to test these kind of inputs if we want to use them across whole Lens.

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented May 14, 2021

This is really strange - I don't get why the tests start failing for a debounce component which isn't even using the new code, but something completely separate.

@flash1293 flash1293 mentioned this pull request May 14, 2021
Merged
@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented May 14, 2021

I can't find the root cause - We could fix it for this PR by reverting the change for the label input (https://github.com/elastic/kibana/pull/100108/files#diff-ea7b153f33d556c9480f5c09e5ff7ca1a6a892b3c2d008a1dfed8368ea0763cb) - this will make the tests pass.

@dej611
Copy link
Copy Markdown
Contributor

dej611 commented May 14, 2021

The advanced editor is using the LabelInput in ranges, but it's also debouncing internally with useDebounceWithOptions. I guess something is generating some conflicts there.
I've tried to clean up some jest timers but could't make it pass yet. Perhaps replacing the internal debounce in the range editor with the new hook may fix it.

@mbondyra
Copy link
Copy Markdown
Contributor Author

mbondyra commented May 17, 2021

@elasticmachine merge upstream

@mbondyra
Copy link
Copy Markdown
Contributor Author

mbondyra commented May 17, 2021

Hey @dej611 you were right that the problem is mixing useDebounce with our debounce. I fixed it by mocking the useDebounce function on the test, let me know your thoughts!

@dej611
Copy link
Copy Markdown
Contributor

dej611 commented May 17, 2021

Mocking it is a good start. It would be nice to address it completely without the mock, but we could delay it as tech debt I guess for now.

@elastic elastic deleted a comment from kibanamachine May 17, 2021
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 17, 2021

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
lens 557 558 +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 1.0MB 1.0MB +141.0B
Unknown metric groups

References to deprecated APIs

id before after diff
canvas 29 25 -4
crossClusterReplication 8 6 -2
fleet 22 20 -2
globalSearch 4 2 -2
indexManagement 12 7 -5
infra 261 149 -112
lens 67 45 -22
licensing 18 15 -3
maps 286 208 -78
ml 121 115 -6
monitoring 109 56 -53
stackAlerts 101 95 -6
total -295

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

flash1293
flash1293 approved these changes May 17, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@flash1293 flash1293 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, only did a code review of the test fix, but the hook works fine for xy extents PR as well

@flash1293
Copy link
Copy Markdown
Contributor

flash1293 commented May 17, 2021

@mbondyra Parts of this PR are in #99203 too - do you want to merge this one first?

@mbondyra mbondyra merged commit 15abf24 into elastic:master May 18, 2021
@kibanamachine kibanamachine mentioned this pull request May 18, 2021
Merged
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 18, 2021
@mbondyra @kibanamachine
[Lens] Debounce axis name inputs mob programming (elastic#100108)
4034a6c
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented May 18, 2021

💚 Backport successful

Status Branch Result
✅ 7.x

This backport PR will be merged automatically after passing CI.

@mbondyra mbondyra deleted the lens/debounce_axis_name_inputs branch May 18, 2021 08:16
kibanamachine added a commit that referenced this pull request May 18, 2021
@kibanamachine @mbondyra
[Lens] Debounce axis name inputs mob programming (#100108) (#100251)
2fa9800 
Co-authored-by: Marta Bondyra <marta.bondyra@gmail.com>
yctercero pushed a commit to yctercero/kibana that referenced this pull request May 25, 2021
@mbondyra @yctercero
[Lens] Debounce axis name inputs mob programming (elastic#100108)
4227e03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dej611 dej611 dej611 approved these changes

@flash1293 flash1293 flash1293 approved these changes

@majagrubic majagrubic Awaiting requested review from majagrubic

Assignees

No one assigned

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 Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v7.14.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Lens] Debounce axis name inputs

5 participants

@mbondyra @elasticmachine @flash1293 @dej611 @kibanamachine