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

Bugfix/scrolling entire page #548

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

dscrobonia
Copy link
Contributor

@dscrobonia dscrobonia commented Jul 11, 2019

fixes a broken fix in #504 for

@dscrobonia dscrobonia requested a review from psiinon July 11, 2019 06:11
@dscrobonia dscrobonia mentioned this pull request Jul 11, 2019
Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

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

Looks good :)

@thc202
Copy link
Member

thc202 commented Jul 11, 2019

Seeing some problems with WebSockets tab:

  1. Go to https://www.websocket.org/echo.html;
  2. Select WebSockets tab;
  3. Connect and send some messages until it starts to scroll (as expected);
  4. Disconnect and connect again;
  5. Send a message and notice that the scroll goes to the top.

@@ -201,26 +227,14 @@ Vue.component('websockets', {

eventBus.$on('updateWebSockets', data => {
this.messages = this.messages.concat(data.messages);
this.scrollToBottom()
Copy link
Member

Choose a reason for hiding this comment

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

This was not added to updateMessages?

@thc202
Copy link
Member

thc202 commented Jul 11, 2019

btw, worth updating the change log with the fix.

@dscrobonia
Copy link
Contributor Author

ah the messageid's on the websocket events aren't unique :( That's why this latest scrolling issue popped up @thc202. Good find!

@dscrobonia
Copy link
Contributor Author

Should work well now.

One behavior with it that I don't like though is that if you were to scroll up to find a specific request, and new request comes in, it will jump you to the bottom. :/ but scrolling is much better now.

@dscrobonia
Copy link
Contributor Author

couldn't figure out how to detect scroll on the table. would be nice to match the behavior of the network tab

Feel free to merge if y'all like the behavior. its probably worth fixing the way it is currently

@psiinon
Copy link
Member

psiinon commented Jul 12, 2019

Hum, that could also be really annoying - jumping to the bottom when you've explicitly scrolled to find something :/
Imagine you're looking at a modern web app that keeps making XHR requests - you'd never be able to find anything before it jumped to the bottom again.

Cant we use scroll events to detect scrolling? https://developer.mozilla.org/en-US/docs/Web/API/Document/scroll_event

I think we want:

  • Normal case, new request comes in we scroll to the bottom
  • User scolls up, new request doesnt scroll
  • User scrolls to the bottom, back to normal case

The other option is something like a checkbox for auto scrolling to the bottom.

Copy link
Member

@psiinon psiinon left a comment

Choose a reason for hiding this comment

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

I think always auto scrolling the the end would make it impossible to find requests with modern web apps :/

@thc202
Copy link
Member

thc202 commented Jul 12, 2019

Yeah, that's the behaviour we have in desktop UI, if the scroll it's at the bottom it keeps scrolling on new entries otherwise it doesn't scroll (the auto-scroll can also be disabled).

@dscrobonia
Copy link
Contributor Author

Yeah that's definitely the behavior we want. I spent a few hours trying to figure that out yesterday and wasn't able to figure out where to tap for the onscroll event. I couldn't find any js hooks or DOM events to see that the user had scrolled. :/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants