Cleanup condensed appendMessage
#1437
Conversation
| if (lastChild && $(lastChild).hasClass("condensed")) { | ||
| lastChild.append(msg); | ||
| condensed.updateText(lastChild, [messageType]); | ||
| } else if (lastChild && $(lastChild).is(constants.condensedTypesQuery)) { |
There was a problem hiding this comment.
Since this is used only once, remove the export and use "." + constants.condensedTypes.join(", .") directly here. I'm not a fan of constants exporting this.
There was a problem hiding this comment.
There's no reason constantly imploding an array into string every time a message is processed.
| } else { | ||
| container.append(msg); | ||
| } | ||
| // TODO: To fix #1432, statusMessage option should entirely be implemented in CSS |
There was a problem hiding this comment.
Yeah, I guess that could simplify the logic here and fix #1432 as well.
| condensed.updateText(newCondensed, [messageType, lastChild.attr("data-type")]); | ||
| container.append(newCondensed); | ||
| newCondensed.append(lastChild); | ||
| newCondensed.append(msg); |
There was a problem hiding this comment.
Any reason why you are changing the order of this?
There was a problem hiding this comment.
- Condensed text will update in memory before it's inserted into DOM.
- Last child will be moved into condensed container without being removed from DOM first.
lol, famous last words. I haven't studied the code in details, but it seems to work fine indeed. |
| @@ -7,7 +7,7 @@ const renderPreview = require("./renderPreview"); | |||
| const utils = require("./utils"); | |||
| const sorting = require("./sorting"); | |||
| const constants = require("./constants"); | |||
| const condense = require("./condensed"); | |||
| const condensed = require("./condensed"); | |||
There was a problem hiding this comment.
I think this was coming from when I couldn't decide whether to say "condense" or "condensed".
| } else { | ||
| container.append(msg); | ||
| } | ||
| // TODO: To fix #1432, statusMessage option should entirely be implemented in CSS |
There was a problem hiding this comment.
Yeah, I guess that could simplify the logic here and fix #1432 as well.
xPaw
added
the
Type: Feature
Cleanup condensed appendMessage
astorije
changed the title
appendMessage
astorije
added
the
Meta: Internal
I have not yet tested this, but it should be logically the same. The PR is open for @YaManicKill to review the code.