Fix date-marker not being removed on loading new messages #1132
Conversation
PolarizedIons
force-pushed
the
fix-datemarker
branch
from
0e1903c
to
f7025de
Compare
|
err. Now yes :) |
|
Looking at your gifts, looks like the viewport jumps a little when content loads now. |
There was a problem hiding this comment.
Woops, sorry!
I mean, pure demonstration of the extra maintenance cost of new features! :(
@xPaw, I would bet that what you pointed at, if indeed an issue, was present in the previous fix made in #765.
I would say let's merge this bugfix and figure out the jump issue separately, but I won't merge this and let you address this the way you want in case you think it should be handled differently.
astorije
added
the
Type: Bug
|
This is the behavior for bc5b03d, before this broke. Not really sure how to fix it either, tbh. |
|
Current master. Does this PR fix this too?
|
client/js/lounge.js
Outdated
| children.eq(1).remove(); | ||
| } | ||
|
|
||
| // get the scrollable wrapper around messages | ||
| var scrollable = chan.closest(".chat"); | ||
| var heightOld = chan.height(); | ||
| var heightOld = chan.height() + dateMarkerHeight; // Add the date marker's height because it was already removed |
There was a problem hiding this comment.
Why don't you just get the height before removing the date marker?
There was a problem hiding this comment.
I guess I was trying to not assume anything, but it is a guarantee that there will be one at the top, so thanks. Moved calculating the height to before I remove it.
PolarizedIons
force-pushed
the
fix-datemarker
branch
from
e2cfa19
to
7da049c
Compare
client/js/lounge.js
Outdated
| children.eq(0).remove(); | ||
| } else if (children.eq(0).hasClass("unread-marker") && children.eq(1).hasClass("date-marker")) { | ||
| // Otherwise the date-marker would get 'stuck' because of the new-message marker | ||
| } else if (children.eq(0).hasClass("unread-marker") && children.eq(1).hasClass("date-marker-container")) { |
There was a problem hiding this comment.
Should that first one check for container too?
There was a problem hiding this comment.
Hmm. I guess it isn't needed, because there will never be normal messages at index 0
PolarizedIons
force-pushed
the
fix-datemarker
branch
from
7da049c
to
c5e215f
Compare
|
Unfortunately, I noticed this is still a bug in
@PolarizedIons, do you think you could take a look? |
|
Ah whoops, simple mistake, though I wish I would stop making them in PR's :\ |
|
It's ok, we all do, and we caught this before releasing :) |
Fix date-marker not being removed on loading new messages
Fixes #763 again. Affects mastrer (5eb5b99). This is caused by 648cfd1, where @astorije put it inside a container, but didn't update the class we searched for.
What it currently looks like:
Expected behavior:
