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

[Logs UI] Fix initial accuracy of logs minimap click #48826

Merged
merged 3 commits into from
Oct 23, 2019

Conversation

Zacqary
Copy link
Contributor

@Zacqary Zacqary commented Oct 21, 2019

Summary

Partial fix for #48614

Fixes the way the log minimap processes clicks. It was previously using a state value to calculate the top of the minimap element, but this state value only gets updated when the user drags the minimap. Therefore, it would return an inaccurate value and jump the user to the wrong time if the user clicked on the minimap before they dragged.

Now, the click handler will get the current DOM top of the minimap element rather than trying to pull it from state.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

@Zacqary Zacqary added bug Fixes for quality problems that affect the customer experience v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 labels Oct 21, 2019
@Zacqary Zacqary requested a review from a team October 21, 2019 21:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui)

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@afgomez
Copy link
Contributor

afgomez commented Oct 22, 2019

I pulled the branch but it still jumps to the incorrect time :/ I'm using the shared 8.0.0 cluster to test.

I see in the description this is a partial fix. What parts would be missing? How can I help?

still-jumps-incorrect

@Zacqary
Copy link
Contributor Author

Zacqary commented Oct 22, 2019

Okay so there's a second problem here: clicking on the minimap is also not sending a new timestamp to the request to fetch new logs, it's still using whatever the current target is. Changing the timestamp in the toolbar does successfully update this.

Not sure where this regression occurred but I'll keep digging.

@Zacqary
Copy link
Contributor Author

Zacqary commented Oct 22, 2019

The issue was stemming from the VerticalScrollPanel reporting the new timestamp and requesting to load additional entries as if the user had scrolled up or down. This was sending a second GraphQL request with the current timestamp instead of the new timestamp.

I made a change to cut down on reports unrelated to scroll events, and that seems to have fixed it.

Although it looks to me as if the scrolling requests aren't working. Both before and after my change, I tried scrolling to the edge of the logs and the Loading additional entries... message never actually yielded any additional entries. Might need to open another issue around that.

Anyway @afgomez as for this being a partial fix; the other part of the issue is a UX issue that would be more complicated to implement, and also has some unanswered questions around it. I just left a comment on the issue explaining further.

@afgomez
Copy link
Contributor

afgomez commented Oct 22, 2019

Cool. Thanks for taking a look 👍

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Zacqary
Copy link
Contributor Author

Zacqary commented Oct 23, 2019

@afgomez Any chance I can get a 👍 on this for review?

Copy link
Contributor

@afgomez afgomez left a comment

Choose a reason for hiding this comment

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

Tested locally and it works 👍

@Zacqary Zacqary merged commit bd3525e into elastic:master Oct 23, 2019
@Zacqary Zacqary deleted the 48614-log-minichart-jumpfix branch October 23, 2019 17:30
@sgrodzicki sgrodzicki added the Feature:Logs UI Logs UI feature label Oct 24, 2019
Zacqary added a commit to Zacqary/kibana that referenced this pull request Oct 24, 2019
* [Logs UI] Fix initial accuracy of logs minimap click

* Fix aberrant request to loadEntriesAfter when using minimap
Zacqary added a commit to Zacqary/kibana that referenced this pull request Oct 24, 2019
* [Logs UI] Fix initial accuracy of logs minimap click

* Fix aberrant request to loadEntriesAfter when using minimap
Zacqary added a commit that referenced this pull request Oct 24, 2019
* [Logs UI] Fix initial accuracy of logs minimap click

* Fix aberrant request to loadEntriesAfter when using minimap
Zacqary added a commit that referenced this pull request Oct 24, 2019
* [Logs UI] Fix initial accuracy of logs minimap click

* Fix aberrant request to loadEntriesAfter when using minimap
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Logs UI Logs UI feature release_note:fix Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services v7.5.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants