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
Completely refactor how date markers are inserted #1452
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.
Fixes the bugs that I encountered and documented in #1450. 👍
Hey @xPaw, unfortunately... |
29f1e61
to
e86e311
Compare
@astorije I refactored it even further, fixed your bug and found another bug which I also fixed. |
46c7c6c
to
32e37b4
Compare
client/js/render.js
Outdated
msg: message | ||
})); | ||
function buildChannelMessages(chanId, chanType, messages) { | ||
return messages.reduce(function(docFragment, message) { |
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.
Nitpick: return messages.reduce((docFragment, message) => {
const msgTime = new Date(msg.time); | ||
|
||
// It's the first message in a window, | ||
// then just append the message and do nothing else |
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.
An idea for a future PR would be to display the date marker if it's the first message in a window and there is no "more" button available. Reason for this would be to remove that odd situation that #1318 shows (I know it's not creating it, just making it more obvious).
return docFragment; | ||
}, $(document.createDocumentFragment())); | ||
} | ||
|
||
function appendMessage(container, chan, chanType, messageType, msg) { | ||
function appendMessage(container, chanId, chanType, msg) { |
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.
This method could use a test or 2, but not blocking this PR obviously.
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.
Tested, works fine.
32e37b4
to
f1a8598
Compare
f1a8598
to
9940042
Compare
Completely refactor how date markers are inserted
Fixes #1450 and most likely other subtle bugs.