Add a date separator to channels/PMs #671

PolarizedIons · 2016-10-04T09:24:01Z

This addresses issue #89 by adding a date separator whenever the date changes between messages. It also adds it to the top of a channel (and before the 'show older messages' button) to indicate the date there.

Previews:

Date Changing:

Private message:

cc @nomadturk

This is also my first big-project PR, so I would appreciate it if you'd tell me if I did something wrong.

PolarizedIons · 2016-10-04T12:03:52Z

Impaloo on irc pointed out that something like "Today"/"Yesterday"/"X days ago" might be better than just displaying the date, at least for recent dates. Any thoughts on this?

MaxLeiter · 2016-10-04T14:09:46Z

Awesome job! It seems like PolarizedIons name is slightly indented in the second photo, any idea about that?

EDIT: just noticed he changed his name between PMs

MaxLeiter

Overall looks great! I made a few small comments but overall think it's a great feature.

However, currently there's no tests or way to disable it - maybe the maintainers have an option on that?

MaxLeiter · 2016-10-04T14:11:03Z

client/js/libs/handlebars/localedate.js

@@ -0,0 +1,5 @@
+"use strict";


Can we not use the already existing localetime somehow?

You mean moving that helper into that file or using the localetime function to get the date? If the latter, that's going to be a bit more work because I'd need to localetime(date).split(",")[0]

I'm fine having these in 2 different helpers, at least for now. When we start messing up with the text itself, then we can use one within the other, but there will still be 2 helpers, so 2 files is ok IMO.

Yeah, this is fine, imo, there is a clear difference between the 2, and we like modularity.

MaxLeiter · 2016-10-04T14:25:11Z

client/js/lounge.js

@@ -372,6 +408,16 @@ $(function() {
 			.find("#chan-" + data.chan)
 			.find(".messages");

+
+			// remove the date-change marker we put at the top, because it may


Please format the comments better - have the top one directly above the bottom, not indented

Sorry that became a thing when switching the spaces to tabs to not get yelled at by the linter

MaxLeiter · 2016-10-04T14:25:29Z

client/js/lounge.js

@@ -384,6 +430,28 @@ $(function() {
 		if (data.messages.length !== 100) {
 			scrollable.find(".show-more").removeClass("show");
 		}
+
+		// Have to use data instiad of the documentFragment because it's being weird


astorije · 2016-10-04T15:30:18Z

First of, thanks a lot for this, very requested feature!! 👏 👏

I will review and test when I am on a stable network, but I wouldn't worry about "humanized" versions for dates as it comes with its set of problems: should it constantly update so that "Today" becomes "Yesterday" when a new day start without refreshing the page? What format? Custom code or package? Should a click on the date do nothing, toggle between "humazined" date and absolute date, append a hash after the URL, etc.
Also, if we're there, I'd rather see "Tuesday 4 October 2016" rather "10/4/2016".

But all that is building up on something that's already an enhancement. I say, let's have a marker that displays the native format outputted by JS (what you did here) when a new day starts (at local midnight) and improve on this later on. This will already make up for a meaty PR to review and test!

williamboman · 2016-10-04T17:07:24Z

should it constantly update so that "Today" becomes "Yesterday" when a new day start without refreshing the page?

setInterval(function () {
    $('[data-relative-time]').forEach(...) // update relative times
}, 60000)

something something :P

nomadturk · 2016-10-04T18:05:59Z

Nice one @stepie22

Looks like it exactly doe what it's supposed to do.

But as for Today, Yesterday tags, I think I'm on the same page with @astorije
We don't actually need them, dates would just do fine. And no need to burden the browsers with more Javascript either. (:

PolarizedIons · 2016-10-04T18:32:49Z

@astorije @nomadturk Thanks for all the kind words :) My first "real" PR, so it's kinda exciting

astorije · 2016-10-05T15:55:53Z

[...] something something :P

This solution is so gross!! 😅
See, this is why we should have a basic version first and build up on it later on ;) No bikeshedding on the details that way :p

We don't actually need them, dates would just do fine.

Well we don't need most of the niceties we have :) I am not opposed to fine-tune the solution, but long-running PRs usually get dropped.

xPaw · 2016-10-07T17:16:22Z

client/js/lounge.js

@@ -1142,13 +1207,34 @@ $(function() {
 			var chan = $(this);
 			if (chan.find(".messages .msg:not(.unread-marker)").slice(0, -100).remove().length) {
 				chan.find(".show-more").addClass("show");
+


I wish we got rid of this damn setInterval, not a fan of running this 'cleanup' code a lot.

@xPaw yea, the loop I made there felt a bit 'hacky', but it was the best solution I could come up with. Had to do this because they would not get removed with those messages. If you have suggestions about what I coded here, feel free to give.

I spent a long time trying to think of a nicer way to do this.

You could obviously select the first child that isn't a date-change-marker, but I don't think there's a way to then delete all except 1 of the date-change-markers before it. And even if you did, jquery would still be doing the same amount of work, so ... meh.

Having said that, I think that this code could be run on initial page load and on each message, rather than on the set interval. We know that the only time this is going to be necessary is if we've had a new message received (or on initial load), don't we?

@YaManicKill I agree it isn't a very good solution to the problem, but it's the best I could find, since it only happens because of the setInterval.

Re: your last paragraph, I feel that that should be its own PR, instead of addressing it here, since I didn't add it, and xPaw has made issue #96 for it.

Ah right, sorry I missed that the issue only happens because the setInterval exists. Alright, Ignore this then, that's a seperate PR.

williamboman · 2016-10-11T08:46:15Z

In highly inactive channels, the date separators look quite bloated (I'm hiding joins/parts/quits e.t.c.)

PolarizedIons · 2016-10-12T18:51:13Z

@williamboman I didn't think of that case. I could add a check to see if the message is visible(which I don't feel is the "right" solution, but think is the best for now), or just not use it at all for join/parts(though I think people would disagree on this).

I think I'll end up adding classes to it, to hide them if the new message would be hidden. If anyone has any thoughts on this, feel free to add

MaxLeiter · 2016-10-12T20:51:41Z

Is anyone sure how IRCCloud handles the date separater? It makes sense to me to only show a separater if there are messages before it unless it's the most recent (meaning in the above screenshot, just 10/11/2016 would show, and a separater would appear after roots message)

Also, should probably hide the date above the topic if you're just joining the channel.

PolarizedIons · 2016-10-18T19:43:51Z

The issue @williamboman has is because of hidden joins/pars as far as I can tell.

Fixing that would be a lot of work/logic, and I don't know how @astorije would feel if I loop through all the messages in a channel the visibility for a message changes to fix the position of the date.

Idealy that part would be reworked so it doesnt trip this up, or the unread marker (same issue with not visible messages)

PolarizedIons · 2016-10-19T10:11:58Z

@MaxLeiter I asked someone using IRCCloud to show what happens with the dates when they hide join/parts. This is what they sent back:

MaxLeiter · 2016-11-22T17:42:16Z

So, seeing as that's expected behavior (compared to IRCcloud), I think this can be merged? (once commits are squashed and such)

williamboman · 2016-11-22T20:39:10Z

I'm 👎 on this one as long as it prints absolute date. (I'd much prefer relative, e.g. Today, Yesterday, 2 days ago and so on)

MiniDigger · 2016-11-22T21:29:32Z

I would also be in favor of displaying the date as x days ago but I also think that it would be a good idea to show the "absolute" date as a tool tip on hover.
(like github does for example )

AlMcKinlay · 2016-11-23T10:10:51Z

I'm 👎 on this one as long as it prints absolute date. (I'd much prefer relative, e.g. Today, Yesterday, 2 days ago and so on)

I agree with @astorije, let's get this one in (it has been up for over a month) and then someone can look at making them relative dates (either as an option or instead of this). I would much rather have this than nothing, and we are dreadful at coming to consensus.

AlMcKinlay

Looks pretty good, I am really excited about this.

Just a few small changes from me, if you implement them, I'll be 100% good on this as a first step for the date marker. This looks great.

AlMcKinlay · 2016-11-23T10:24:31Z

client/js/lounge.js

@@ -1142,13 +1207,34 @@ $(function() {
 			var chan = $(this);
 			if (chan.find(".messages .msg:not(.unread-marker)").slice(0, -100).remove().length) {
 				chan.find(".show-more").addClass("show");
+


I spent a long time trying to think of a nicer way to do this.

You could obviously select the first child that isn't a date-change-marker, but I don't think there's a way to then delete all except 1 of the date-change-markers before it. And even if you did, jquery would still be doing the same amount of work, so ... meh.

Having said that, I think that this code could be run on initial page load and on each message, rather than on the set interval. We know that the only time this is going to be necessary is if we've had a new message received (or on initial load), don't we?

AlMcKinlay · 2016-11-23T10:25:18Z

client/views/date-change-marker.tpl

@@ -0,0 +1,3 @@
+<div class="date-change-marker">


Minor nitpick, can we just call it "date-marker"? No need for the "change" (especially as the first date marker on the screen won't be about the date changing, it's about telling you the original date).

AlMcKinlay · 2016-11-23T10:26:14Z

client/js/libs/handlebars/localedate.js

@@ -0,0 +1,5 @@
+"use strict";


Yeah, this is fine, imo, there is a clear difference between the 2, and we like modularity.

PolarizedIons · 2016-11-23T12:45:17Z

@williamboman I agree with @astorije and @YaManicKill, this PR has definitely dragged on long enough. I'm sure you can whip up a quick PR up to make it use relative 'dates' :)

PolarizedIons · 2016-11-23T14:29:28Z

I've gone ahead and squashed this (with the help of YaManiac)

AlMcKinlay · 2016-11-24T09:36:31Z

So, we have 2 approvals on this.

@astorije You mentioned you were going to have a look at it. Do you feel you want to check it before it's merged? I'm guessing you are probably fine, but let me know if you aren't.

@williamboman Are you actually 👎 on this as it is? Or are you fine with it, on the assumption that someone else will do relative dates sometime.

williamboman · 2016-11-24T12:15:25Z

@williamboman Are you actually 👎 on this as it is? Or are you fine with it, on the assumption that someone else will do relative dates sometime.

I'm fine with it as it is ^{but pls ^{rel dates someone?}}

astorije · 2016-11-27T21:32:22Z

@astorije You mentioned you were going to have a look at it. Do you feel you want to check it before it's merged? I'm guessing you are probably fine, but let me know if you aren't.

Sorry, work has kept me busy enough that sleeping has become entirely optional now 😅
Thanks for carefully reviewing this, and I'm happy this got merged! Good job @stepie22!

Add a date separator to channels/PMs

MaxLeiter reviewed Oct 4, 2016

View reviewed changes

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Oct 4, 2016

xPaw reviewed Oct 7, 2016

View reviewed changes

AlMcKinlay requested changes Nov 23, 2016

View reviewed changes

AlMcKinlay approved these changes Nov 23, 2016

View reviewed changes

AlMcKinlay force-pushed the date-seperator branch from 0262df4 to 4fe438b Compare November 23, 2016 14:17

PolarizedIons force-pushed the date-seperator branch from 4fe438b to 08d3d8b Compare November 23, 2016 14:28

Add a date seperator

93f0f69

PolarizedIons force-pushed the date-seperator branch from 08d3d8b to 93f0f69 Compare November 23, 2016 14:35

xPaw approved these changes Nov 23, 2016

View reviewed changes

williamboman approved these changes Nov 24, 2016

View reviewed changes

xPaw added this to the 2.2.0 milestone Nov 24, 2016

xPaw merged commit d695c64 into thelounge:master Nov 24, 2016

astorije mentioned this pull request Apr 19, 2017

Use moment on the client to display friendly dates #1054

Merged

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017

Merge pull request thelounge#671 from stepie22/date-seperator

89a2f46

Add a date separator to channels/PMs

Add a date separator to channels/PMs #671

Add a date separator to channels/PMs #671

Conversation

PolarizedIons commented Oct 4, 2016

PolarizedIons commented Oct 4, 2016

MaxLeiter commented Oct 4, 2016 • edited Loading

MaxLeiter left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Oct 4, 2016 • edited Loading

williamboman commented Oct 4, 2016

nomadturk commented Oct 4, 2016

PolarizedIons commented Oct 4, 2016

astorije commented Oct 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamboman commented Oct 11, 2016

PolarizedIons commented Oct 12, 2016

MaxLeiter commented Oct 12, 2016

PolarizedIons commented Oct 18, 2016

PolarizedIons commented Oct 19, 2016 • edited Loading

MaxLeiter commented Nov 22, 2016 • edited Loading

williamboman commented Nov 22, 2016

MiniDigger commented Nov 22, 2016

AlMcKinlay commented Nov 23, 2016

AlMcKinlay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PolarizedIons commented Nov 23, 2016

PolarizedIons commented Nov 23, 2016

AlMcKinlay commented Nov 24, 2016

williamboman commented Nov 24, 2016 • edited Loading

astorije commented Nov 27, 2016

MaxLeiter commented Oct 4, 2016 •

edited

Loading

astorije commented Oct 4, 2016 •

edited

Loading

PolarizedIons commented Oct 19, 2016 •

edited

Loading

MaxLeiter commented Nov 22, 2016 •

edited

Loading

williamboman commented Nov 24, 2016 •

edited

Loading