Show seconds in timestamp #1141

bews · 2017-05-06T19:07:45Z

Just a simple option in settings (disabled by default):

Here how it looks:

Related issue: #98

MaxLeiter · 2017-05-06T20:45:13Z

not looking at code, am fan of feature 👍

astorije · 2017-05-06T23:19:57Z

I think it would be even better if we could set the date format ourself

We have discussed this in other tickets, this is not something we will do as part of the main codebase (what a nice way to say "packages!" until we get to that 😅). I invite you to look for them for rationale, I'm on a connection that's too spotty to browser around, sorry. Essentially, 12/24h + show seconds checkboxes will cover 95-99% of use cases while not having to learn a syntax for that (in fact, most if not all OS settings offer the same) so checkboxes = ease of use.

@bews, we should be able to get that out with the next release (v2.4.0) once reviewed, thanks! 😃

xPaw · 2017-05-07T08:45:59Z

client/views/msg.tpl

@@ -1,6 +1,6 @@
 <div class="msg {{type}}{{#if self}} self{{/if}}{{#if highlight}} highlight{{/if}}" id="msg-{{id}}" data-time="{{time}}" data-from="{{from}}">
-	<span class="time tooltipped tooltipped-e" aria-label="{{localetime time}}">
-		{{tz time}}
+	<span class="time tooltipped tooltipped-e {{#if seconds}}seconds{{/if}}" aria-label="{{localetime time}}">


I think it's better to set option classes on body, like option-seconds. How would this mix with 12h+seconds (01:11:11 PM) display anyway? Perhaps it's better to make it a flexible column, if possible.

How would this mix with 12h+seconds (01:11:11 PM) display anyway?

Is it a planned feature or I'm missing smth?

I think he's just meaning as a future plan, trying to think about how this would interoperate with it.

If we cared about it updating correctly when we change the settings (currently if you change the settings only new messages will have the correct format) then we could set it on the body (or container) and then have both time formats rendered, and show/hide in css based on the parameter we set on the container. That doesn't reaaaaally work well if we end up having multiple settings, and maybe feels a bit overkill (it doubles the number of time spans we have, with half of them being hidden). Let's leave that for now, this is good enough for just now I think. This is a setting we should have had a long time ago, personally.

AlMcKinlay

Looks good to me, just a couple of small things. Thanks for this, @bews, this is well overdue, and generally a nice PR.

AlMcKinlay · 2017-05-08T08:10:16Z

client/js/libs/handlebars/tz.js

-module.exports = function(time) {
-	return moment(time).format("HH:mm");
+module.exports = function(time, seconds) {
+	return seconds ? moment(time).format("HH:mm:ss") : moment(time).format("HH:mm");


Can I be a code pedant and request something like this:

const format = seconds ? "HH:mm:ss" : "HH:mm"; return moment(time).format(format);

I would also prefer if the formats were in constants somewhere, but I won't push that one too hard.

AlMcKinlay · 2017-05-08T08:11:31Z

client/js/libs/handlebars/tz.js

@@ -2,6 +2,6 @@

 const moment = require("moment");

-module.exports = function(time) {
-	return moment(time).format("HH:mm");
+module.exports = function(time, seconds) {


There's something a little bit weird about passing through seconds here, but I don't know what the better option would be, tbh. If we had control over requring these handlebars templates, then we could use the modules that we are creating, but webpack pulls them in itself, so we'll end up with differing settings, which isn't good. So, I guess this is just me moaning about the system rather than asking you to do anything.

AlMcKinlay · 2017-05-08T08:14:23Z

client/views/msg.tpl

@@ -1,6 +1,6 @@
 <div class="msg {{type}}{{#if self}} self{{/if}}{{#if highlight}} highlight{{/if}}" id="msg-{{id}}" data-time="{{time}}" data-from="{{from}}">
-	<span class="time tooltipped tooltipped-e" aria-label="{{localetime time}}">
-		{{tz time}}
+	<span class="time tooltipped tooltipped-e {{#if seconds}}seconds{{/if}}" aria-label="{{localetime time}}">


If we cared about it updating correctly when we change the settings (currently if you change the settings only new messages will have the correct format) then we could set it on the body (or container) and then have both time formats rendered, and show/hide in css based on the parameter we set on the container. That doesn't reaaaaally work well if we end up having multiple settings, and maybe feels a bit overkill (it doubles the number of time spans we have, with half of them being hidden). Let's leave that for now, this is good enough for just now I think. This is a setting we should have had a long time ago, personally.

AlMcKinlay · 2017-05-08T08:15:08Z

client/js/options.js

@@ -19,6 +19,7 @@ const options = $.extend({
 	notifyAllMessages: false,
 	part: true,
 	quit: true,
+	seconds: false,


Maybe change this to "showSeconds" because it makes sense here, but everywhere else (data.msg.seconds) it isn't clear exactly what it does.

bews · 2017-05-09T04:34:40Z

So, I refactored this thing and now there even less code 😄

Decided to keep the previous commit for now to compare.

Here how it looks after the changes:

xPaw · 2017-05-09T07:53:41Z

client/js/libs/handlebars/tz.js

@@ -1,7 +1,9 @@
 "use strict";

 const moment = require("moment");
+const options = require("../../options");


Does this happen to register options stuff twice? That code isn't so modular to be required multiple times afaik.

Does this happen to register options stuff twice?

Just checked - it doesn't.

So, I know for certain that node caches require, so that it only runs the stuff once, and you get the same object back if you require it multiple times. So that's exactly how the options module was designed, so that we can require it anywhere within the client code.

I'm not 100% sure on that with webpack, but if @bews says it works, then I guess it does.

I'll do some testing of this before. I thought it might not work just because the handlebars templates are included into webpack in a separate way, so I just assumed it wouldn't. If this works, it's definitely better than the old way.

AlMcKinlay

I'll test this later to make sure I'm happy with how this works. But assuming it works the way I expect it to, it's ready to go. Hopefully I'll have some time today or tomorrow to check.

AlMcKinlay · 2017-05-09T09:36:10Z

client/js/libs/handlebars/tz.js

@@ -1,7 +1,9 @@
 "use strict";

 const moment = require("moment");
+const options = require("../../options");


So, I know for certain that node caches require, so that it only runs the stuff once, and you get the same object back if you require it multiple times. So that's exactly how the options module was designed, so that we can require it anywhere within the client code.

I'm not 100% sure on that with webpack, but if @bews says it works, then I guess it does.

I'll do some testing of this before. I thought it might not work just because the handlebars templates are included into webpack in a separate way, so I just assumed it wouldn't. If this works, it's definitely better than the old way.

astorije · 2017-05-09T22:33:09Z

@xPaw, any idea how we could make it work with existing messages? Sorry @YaManicKill but your suggestion is... ahem, less than ideal 😅
Each .msg line has a data-time, so we could recompute the timestamp from that and update the DOM for everything. Not ideal either.

The more we move forward with client, the more I think our HTML should be the rendered view of a state we keep in JS. Right now the state is the DOM, and this has been biting us quite a bit.

AlMcKinlay · 2017-05-10T08:06:17Z

Well I guess here's a better (and still css related) options.

Wrap the seconds in a span that is shown/hidden appropriately. So we always generate the format as "HH:mm:ss" but the ":ss" is inside another span.

The only other way I can think of is going through them all to change the format when we change the setting. Which doesn't feel idea.

xPaw · 2017-05-13T10:55:20Z

Hiding seconds in Dom approach won't work if we want to also support 12H format. Re-rendering should be a couple lines of code.

astorije · 2017-05-13T18:45:42Z

Agreed with @xPaw. Re-rendering is not going to be that expensive, and it's an option you change once and keep forever (or like, until your personal preferences change), not something you'd change every day.

AlMcKinlay · 2017-05-15T07:54:49Z

Yeah, fine, I guess you are right. Re-rendering all the times when the user changes the settings sounds like the best idea and won't be too difficult. @bews are you happy to add this to the PR? I'll be happy with the PR if that is done.

bews · 2017-05-15T09:07:03Z

Yep, will do that later.

bews · 2017-05-16T09:49:13Z

Done.

xPaw · 2017-05-17T08:08:19Z

Do you want to update this PR to support 12h (am/pm) as well?

xPaw · 2017-05-17T08:11:05Z

client/js/options.js

+	} else if (name === "showSeconds") {
+		var format = options.showSeconds ? constants.timeFormats.msgWithSeconds : constants.timeFormats.msgDefault;
+		chat.find(".msg > .time").each(function() {
+			$(this).text(moment($(this).parent().data("time")).format(format));


Call the TS helper here, then you don't need duplicate code.

Call the TS helper here, then you don't need duplicate code.

TZ you mean? I can't, since it imports options now.

There's a fix for that. Move the import into the function on the export (so it only gets callled when the function gets called) and then add the import for the helper here. It's a little less efficient, but there should be no cyclical dependency.

AlMcKinlay · 2017-05-17T08:32:03Z

Do you want to update this PR to support 12h (am/pm) as well?

Let's leave that to another PR.

AlMcKinlay

Looks good to me.

xPaw · 2017-06-09T07:57:55Z

client/js/options.js

@@ -62,7 +64,7 @@ settings.on("change", "input, select, textarea", function() {
 		"nick",
 		"part",
 		"quit",
-		"notifyAllMessages",
+		"notifyAllMessages"


Revert this line.

xPaw · 2017-06-09T07:58:11Z

client/js/libs/handlebars/tz.js


 module.exports = function(time) {
-	return moment(time).format("HH:mm");
+	const options = require("../../options");


Did this require outside of the function not work?

No, because otherwise we have a cyclical dependency, with options requiring this file.

xPaw · 2017-06-10T11:49:05Z

Could you squash the commits? I will merge after that.

bews · 2017-06-10T11:53:50Z

done

Show seconds in timestamp

astorije · 2018-04-28T05:36:49Z

Hey @bews, we have free sticker packs for our contributors now!
If you're interested, go fill the form at https://goo.gl/forms/f5usqAEp5DWoeXC92 🙂

astorije added this to the 2.4.0 milestone May 6, 2017

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label May 6, 2017

xPaw reviewed May 7, 2017

View reviewed changes

AlMcKinlay requested changes May 8, 2017

View reviewed changes

xPaw reviewed May 9, 2017

View reviewed changes

AlMcKinlay reviewed May 9, 2017

View reviewed changes

bews mentioned this pull request May 15, 2017

Condense join into nicknames #1047

Closed

bews added a commit to bews/lounge that referenced this pull request May 16, 2017

Condensed messages: thelounge#1141 conflict fix

008b27a

xPaw reviewed May 17, 2017

View reviewed changes

AlMcKinlay approved these changes Jun 5, 2017

View reviewed changes

xPaw reviewed Jun 9, 2017

View reviewed changes

bews force-pushed the bews/dev-6-seconds branch from 0cb6a5f to 70c95e3 Compare June 10, 2017 11:26

xPaw approved these changes Jun 10, 2017

View reviewed changes

Show seconds in timestamp

fd983a7

bews force-pushed the bews/dev-6-seconds branch from 70c95e3 to fd983a7 Compare June 10, 2017 11:53

xPaw merged commit 53ffcb5 into thelounge:master Jun 10, 2017

xPaw modified the milestones: 2.3.2, 2.4.0 Jun 10, 2017

xPaw mentioned this pull request Jun 10, 2017

Timestamp Show Seconds #98

Closed

bews deleted the bews/dev-6-seconds branch June 10, 2017 17:32

amaranth pushed a commit to amaranth/lounge that referenced this pull request Jun 14, 2017

Condensed messages: thelounge#1141 conflict fix

3e0e05f

AlMcKinlay mentioned this pull request Jun 23, 2017

Initial status message condensing #759

Merged

15 tasks

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

Merge pull request thelounge#1141 from bews/bews/dev-6-seconds

e3fd31a

Show seconds in timestamp

fnutt mentioned this pull request Sep 12, 2018

Add an option to display 12hour times #2770

Closed

Show seconds in timestamp #1141

Show seconds in timestamp #1141

Conversation

bews commented May 6, 2017

MaxLeiter commented May 6, 2017

astorije commented May 6, 2017

xPaw May 7, 2017 • edited Loading

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

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

Choose a reason for hiding this comment

bews commented May 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlMcKinlay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented May 9, 2017

AlMcKinlay commented May 10, 2017

xPaw commented May 13, 2017

astorije commented May 13, 2017

AlMcKinlay commented May 15, 2017

bews commented May 15, 2017

bews commented May 16, 2017

xPaw commented May 17, 2017 • edited Loading

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

AlMcKinlay commented May 17, 2017

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

Choose a reason for hiding this comment

xPaw commented Jun 10, 2017

bews commented Jun 10, 2017

astorije commented Apr 28, 2018

xPaw May 7, 2017 •

edited

Loading

bews commented May 9, 2017 •

edited

Loading

xPaw commented May 17, 2017 •

edited

Loading