-
Notifications
You must be signed in to change notification settings - Fork 687
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
Always create condensed wrapper #1485
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems sensible to me.
const msgTime = new Date(msg.time); | ||
|
||
// It's the first message in a window, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes, this would be an issue after removing the logic for not creating a condense if there is only 1 message, as it woudl never be created until we had a normal message.
client/js/render.js
Outdated
// If date changed, we don't need to do condensed logic | ||
container.append(renderedMessage); | ||
return; | ||
lastChild = $(templates.date_marker({msgDate: msgTime})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this could be the issue. It's certainly not a great plan just now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that's a very confusing sentence.
I do this the code that was here could be the issue we were seeing with @Jay2k1 and what we were doing doesn't actually make any sense to do, because we are already checking if the last thing is a condensed message anyway.
But I have just realised why this won't work actually.
The date marker isn't a ".msg". So we'll miss the date marker, condense into the previous condensed, and the order will be wrong, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is possible.. I will look further into it later.
e60d4c9
to
1629f02
Compare
@YaManicKill I think I fixed the issue, and simplified the code a bit, so there's no longer need to check for parent element. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trusting @YaManicKill (and you @xPaw obviously 😅) on that one.
One linting error to fix though:
37:8 error 'parent' is assigned a value but never used no-unused-vars
Also @YaManicKill could you give this another try now that @xPaw has addressed the issue?
1629f02
to
b86a4dd
Compare
Nevermind, fixed that for ya. @YaManicKill, feel free to merge if your test appears conclusive. |
b86a4dd
to
94d4025
Compare
Always create condensed wrapper
It would not create a wrapper for new channels and when the date changed.