Fix scrolling in chat view#838
Merged
Merged
Conversation
When an element has a negative top margin jQuery returns its top position as the position in where the element would be if it had no margin. In a similar way, when there is a negative margin the returned height is not the actual height of the element, but its height without that margin. The message list is expected to scroll to the newest message if the last one is partially visible, but due to the jQuery behaviour the message list did not scroll when the last message was grouped and less than 20px (the negative top margin) of the content of that message was visible. Now the negative margin is taken into account and the message list always scrolls to the newest message if the last one is partially visible. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
The scroll area covers the top and bottom padding of an element, but not its top and bottom margins, so the margins should not be taken into account in the scroll calculations. Currently this does not cause any difference in the behaviour, as the message lists does not have any margin, but it is fixed just in case it is added in the future. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
When an element is detached and then attached again its scroll position is reset to the top. Now the chat view provides the "saveScrollPosition" and "restoreScrollPosition" methods to save the scroll position before detaching its element and then restoring it when attached again. Note that the position can not be exactly restored, as the size of the chat view may have changed due to switching it from the main view to the sidebar, and thus the visible messages can not be the same in both cases. Due to this, restoring the scroll position just ensures that the last message that was partially visible when it was saved will be fully visible when it is restored. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
4 tasks
Member
|
Ah, after reading the code I see, you go by the last comment, not the first |
nickvergessen
approved these changes
May 2, 2018
Member
Author
|
The acceptance test failure is a false positive. I will fix it in another pull request (as it is not related to this one; it is just a timing issue). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #564.
This pull request fixes the chat view not scrolling to the new message if the last message was grouped and partially visible. More important, it also fixes the scroll position not being kept when the chat view is switched between the main view and the sidebar.
How to test (chat view not scrolling to new message):
Expected result:
Actual result:
How to test (scroll position kept when switching the chat view):
Expected result:
Actual result: