Add a relative prefix to the date divider in the chat #591

nickvergessen · 2018-01-15T15:17:46Z

@jancborchardt it's not exactly the steps you mentioned, but instead its automatically done by momentjs.
I think it's good enough, but since "a month ago" is repeating with each day in the past, I'd also be fine with only showing relative until it's no longer displayed as "days ago".
No one needs relative dates for "7 months ago".

Fix #559

danxuliu

Besides the code comments, the relative dates should be automatically updated when changing to a new day.

danxuliu · 2018-01-15T15:54:16Z

js/views/chatview.js

-			if (this._lastAddedMessageModel && !this._modelsHaveSameDate(this._lastAddedMessageModel, model)) {
-				// 'LL' formats a localized date including day of month, month
-				// name and year
+			if (!this._lastAddedMessageModel || !this._modelsHaveSameDate(this._lastAddedMessageModel, model)) {


Just to be sure, the condition was changed to make the date divider appear before the first message, right?

danxuliu · 2018-01-15T15:55:43Z

js/views/chatview.js

 				if (this._oldestOnTopLayout) {
-					$el.attr('data-date', OC.Util.formatDate(model.get('date'), 'LL'));
+					$el.attr('data-date', this._getDateSeparator(model.get('date')));
 					$el.addClass('showDate');
 				} else {


Due to the change in the outer if condition it must be checked here that there is a this._lastAddedMessageModel (otherwise the line below would fail).

danxuliu · 2018-01-15T16:00:18Z

js/views/chatview.js

+			}
+
+
+			return relativePrefix + ', ' + OC.Util.formatDate(timestamp, 'LL');


The whole text should be internationalized, as depending on the locale a different separator (or even order of the prefix and date) could be needed.

Also please add a comment like 'LL' formats a localized date including day of month, month name and year to know what it does ;-)

danxuliu · 2018-01-15T16:08:09Z

js/views/chatview.js

+			var date = moment(timestamp, 'x'),
+				today = moment(),
+				dayOfYear = OC.Util.formatDate(date, 'YYYY-DDD'),
+				dateOfToday = OC.Util.formatDate(today, 'YYYY-DDD');


dateOfToday -> currentDayOfYear/dayOfYearNow/dayOfYearToday/whatever including dayOfYear ;-)

danxuliu · 2018-01-15T16:22:03Z

js/views/chatview.js

@@ -242,6 +240,29 @@
 			}
 		},

+		_getDateSeparator: function(timestamp) {
+			var date = moment(timestamp, 'x'),
+				today = moment(),


Maybe it would be better to call it now instead of today? Not really sure which one would be clearer, though.

jancborchardt · 2018-01-15T20:17:48Z

Looks nice! :) I think the relative dates of even the multiple months ago are useful when you scroll back.

@nickvergessen so now that the date display on the right of each message duplicates the divider info, can we just show the timestamp of the specific message there on the right? Then that part also has a shorter and predictable width.

nickvergessen · 2018-01-16T10:23:50Z

We are using relative dates now for the first 24h, afterwards it switches to Time (or should we include the date too, because the date divider might not be visible?
Or can we pin the last date divider with some css magic?

Signed-off-by: Joas Schilling <coding@schilljs.com>

When using the oldest to newest layout now the first (oldest) message always shows the date. When using the newest to oldest layout, however, there is no date shown in the first (newest) message; in this case it would be good to show it only if the newest message was not sent today, but the newest to oldest layout is currently not used anywhere (and it is likely to be removed from the code), so there was no need to spend time on that. Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

Signed-off-by: Joas Schilling <coding@schilljs.com>

danxuliu · 2018-01-17T01:13:48Z

I have rewritten a little the commit history to fix a lint issue, add the review changes to the original commit and split adding the relative prefix from showing the date in the first message.

However, there are still some issues. Although the relative date is not set initially due to the use of the live-relative-timestamp CSS class it will be set anyway after 30 seconds. Also, the relative date in the header is not automatically updated, so when chatting near midnight you will end with two Today headers. Probably it will be needed to use a custom function to update the relative time akin to the one used in the server.

@nickvergessen

should we include the date too, because the date divider might not be visible?

Not sure about the date; it may be too much visually, and if you need to find the date you can hover on the time... or, in touch screens, scroll back until you find the date header. That could be quite annoying, though, so I do not know.

In any case I would add back the seconds, at the very least to the tooltip and maybe to the text itself too, as otherwise it would not be possible to know them in touch screens.

Or can we pin the last date divider with some css magic?

I am not aware of any CSS-only way to pin the last divider and, although it could be a cool effect, for the time being I would not add it to keep the code simple (there are enough bugs to fix already ;-) ).

nickvergessen · 2018-01-17T09:28:22Z

Fixed the issue with live-relative-timestamp

In any case I would add back the seconds, at the very least to the tooltip and maybe to the text itself too, as otherwise it would not be possible to know them in touch screens.

Done

I am not aware of any CSS-only way to pin the last divider and, although it could be a cool effect, for the time being I would not add it to keep the code simple (there are enough bugs to fix already ;-) ).

Yeah, that's why I asked for some simple CSS, lets just keep it like that for now then.

Signed-off-by: Joas Schilling <coding@schilljs.com>

danxuliu

Blocking to prevent a merge, as dates in the headers are not updated yet when the day changes.

nickvergessen · 2018-01-17T11:59:12Z

What do you mean?
after midnight it still says 1 day ago, although it should be 2 days ago now?

nickvergessen · 2018-01-17T12:05:13Z

Merging as discussed

Close the datepicker on unfocus

nickvergessen added 3. to review bug design labels Jan 15, 2018

nickvergessen added this to the 3.1 (Nextcloud 13.0.2/3) milestone Jan 15, 2018

nickvergessen requested review from jancborchardt and danxuliu January 15, 2018 15:17

danxuliu reviewed Jan 15, 2018

View reviewed changes

nickvergessen and others added 3 commits January 16, 2018 18:04

Add a relative prefix to the date divider in the chat

4826207

Signed-off-by: Joas Schilling <coding@schilljs.com>

No relative date on the comment after a day, makes no sense...

af8e1aa

Signed-off-by: Joas Schilling <coding@schilljs.com>

danxuliu force-pushed the bugfix/559/date-divider-improvements branch from 46e43c6 to af8e1aa Compare January 16, 2018 20:44

Do not update non-relative dates and show seconds again

a86d12b

Signed-off-by: Joas Schilling <coding@schilljs.com>

Ivansss approved these changes Jan 17, 2018

View reviewed changes

danxuliu requested changes Jan 17, 2018

View reviewed changes

nickvergessen merged commit 8f88fe8 into stable13 Jan 17, 2018

nickvergessen deleted the bugfix/559/date-divider-improvements branch January 17, 2018 12:05

nickvergessen mentioned this pull request Feb 13, 2018

Prep release of 3.1.0 #656

Merged

marcoambrosini pushed a commit that referenced this pull request Oct 9, 2019

Close the datepicker on unfocus (#591)

563f576

Close the datepicker on unfocus

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a relative prefix to the date divider in the chat #591

Add a relative prefix to the date divider in the chat #591

nickvergessen commented Jan 15, 2018 •

edited

Loading

danxuliu left a comment

danxuliu Jan 15, 2018

danxuliu Jan 15, 2018

danxuliu Jan 15, 2018

danxuliu Jan 15, 2018

danxuliu Jan 15, 2018

jancborchardt commented Jan 15, 2018

nickvergessen commented Jan 16, 2018

danxuliu commented Jan 17, 2018 •

edited

Loading

nickvergessen commented Jan 17, 2018

danxuliu left a comment

nickvergessen commented Jan 17, 2018

nickvergessen commented Jan 17, 2018

		}


		return relativePrefix + ', ' + OC.Util.formatDate(timestamp, 'LL');

Add a relative prefix to the date divider in the chat #591

Add a relative prefix to the date divider in the chat #591

Conversation

nickvergessen commented Jan 15, 2018 • edited Loading

danxuliu left a comment

Choose a reason for hiding this comment

danxuliu Jan 15, 2018

Choose a reason for hiding this comment

danxuliu Jan 15, 2018

Choose a reason for hiding this comment

danxuliu Jan 15, 2018

Choose a reason for hiding this comment

danxuliu Jan 15, 2018

Choose a reason for hiding this comment

danxuliu Jan 15, 2018

Choose a reason for hiding this comment

jancborchardt commented Jan 15, 2018

nickvergessen commented Jan 16, 2018

danxuliu commented Jan 17, 2018 • edited Loading

nickvergessen commented Jan 17, 2018

danxuliu left a comment

Choose a reason for hiding this comment

nickvergessen commented Jan 17, 2018

nickvergessen commented Jan 17, 2018

nickvergessen commented Jan 15, 2018 •

edited

Loading

danxuliu commented Jan 17, 2018 •

edited

Loading