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

Fix requesting last messages when no message id is known #1519

Merged
merged 1 commit into from Sep 14, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Sep 12, 2017

Fixes #1460.

However there's another bug. When doing /clear, the show more button jumps into view, thus IntersectionObserver calls observe on the button, and it loads last history again.

@xPaw xPaw added Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. Meta: Do Not Merge This PR should not be merged. labels Sep 12, 2017
@xPaw xPaw added this to the 2.5.0 milestone Sep 12, 2017
@AlMcKinlay
Copy link
Member

When doing /clear, the show more button jumps into view, thus IntersectionObserver calls observe on the button

Hahahaha. That's brilliant.

But that does lead us to a question ... what is the purpose of clear?

I would say either it would clear both client and server side, and therefore have no way to get the stuff back, or it is a pointless feature. Thoughts?

@xPaw
Copy link
Member Author

xPaw commented Sep 12, 2017

I don't know either, it sounds like it should clear server buffer too.

@astorije
Copy link
Member

what is the purpose of clear?

/clear really acts like clear/ctrl+L in the terminal: it hides / empties the screen, whose content can be brought back by scrolling up.
I actually use it (infrequently) on The Lounge, but I would say a good 60% of the usage I'm making of it can now be achieved by typing /collapse.

I have tried a few things to reproduce this, and nothing worked. Ideally, we would want to be able to set the scrollTop past the maximum, so that latest message is right before the visible block but as far as I know, it's not possible on the web. I have seen a couple "clearable" apps that did that nicely, but they were all native.

Because this is not doable (unless someone has a genius idea), I'd be in favor of removing it.
In fact, even though I use the command myself, I would not like its action to be changed from "hide the latest message" to "destroy all messages in the current channel". That would be a harsh change of an existing feature 😅.
There should be a way to wipe out all the logs for a given channel (in fact, there might be an open issue about this) in a context menu entry for example, but this is a very destructive action that shouldn't be possible with an unfortunate shortcut/command accident lol.

So yeah, until then, I think we should just remove it if there is no way to fix it without drastically changing its behavior.

@xPaw
Copy link
Member Author

xPaw commented Sep 13, 2017

Regardless of this clear command bug, this PR fixes the issue on its own, the fact it loads history on clear is a different story, so this can be merged.

@astorije
Copy link
Member

That's fair. Is the do not merge label still relevant?

@xPaw xPaw removed the Meta: Do Not Merge This PR should not be merged. label Sep 14, 2017
@AlMcKinlay AlMcKinlay merged commit 171449c into master Sep 14, 2017
@AlMcKinlay AlMcKinlay deleted the xpaw/fix-show-more-when-empty branch September 14, 2017 06:25
@astorije astorije changed the title Fix requesting last messages when no message id is known Fix requesting last messages when no message id is known Sep 29, 2017
@AlMcKinlay AlMcKinlay removed their assignment Mar 12, 2018
@xPaw xPaw removed their assignment Mar 12, 2018
@xPaw xPaw mentioned this pull request Feb 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants