Kierangould/12hourtimestamp #903

Kieran-Gould · 2017-05-18T21:21:22Z

Added support for 12 hour timestamps. Option for 12/24hr is pulled from UserSettings.

matrixbot · 2017-05-18T21:21:24Z

Can one of the admins verify this patch?

ara4n · 2017-05-18T22:00:39Z

src/DateUtils.js

+  let ampm = hours >= 12 ? 'PM' : 'AM';
+  hours = hours % 12;
+  hours = hours ? hours : 12;
+  minutes = minutes < 10 ? '0'+minutes : minutes;


i suggest just using the pad(n) function defined above :)

ara4n · 2017-05-18T22:01:15Z

src/DateUtils.js


 function pad(n) {
    return (n < 10 ? '0' : '') + n;
 }

+function twentyfourhour(date) {


please indent consistently as per the code-style (i.e. 4 spaces). The function probably should be twentyFourHour() too for consistent casing.

ara4n · 2017-05-18T22:02:03Z

src/components/structures/UserSettings.js

@@ -65,7 +65,6 @@ const SETTINGS_LABELS = [
        id: 'dontSendTypingNotifications',
        label: "Don't send typing notifications",
    },
-/*


ara4n · 2017-05-18T22:06:24Z

src/components/views/rooms/EventTile.js

@@ -479,24 +481,31 @@ module.exports = WithMatrixClient(React.createClass({
        );

        var e2e;
+        let e2e_style;


Hm. Changing the whole CSS to make room for 24h timestamps feels a bit worrying, although I see why you're doing it...

A nicer bet here would be to just put the mx_EventTile_12h class onto the EventTile itself, and then in the CSS have a selector like:

.mx_EventTile_12h .mx_EventTile_e2eIcon { padding-left: 5px; } .mx_EventTile_12h .mx_EventTile_line { padding-left: 100px; }

...or whatever the tweaks are. And then you don't need all these overrides of the e2e_style, which are quite confusing at first as it makes it sound like the e2e icon is somehow very explicitly being changed by the timestamp(!) rather than just for layout purposes.

TL;DR: cascading stylesheets cascade; let's make the most of that :)

t3chguy · 2017-05-18T21:28:10Z

src/DateUtils.js

+import UserSettingsStore from './UserSettingsStore';
+const days = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"];
+const months = ["Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"];
+let time;


I may be being blind but where is time referenced?

t3chguy · 2017-05-18T21:39:23Z

src/DateUtils.js

+function twentyfourhour(date) {
+  let hours = date.getHours();
+  let minutes = date.getMinutes();
+  let ampm = hours >= 12 ? 'PM' : 'AM';


this can be const

t3chguy · 2017-05-18T21:43:37Z

src/components/views/rooms/EventTile.js

@@ -16,6 +16,8 @@ limitations under the License.

 'use strict';

+import UserSettingsStore from '../../../UserSettingsStore';


this may be potentially be better being passed into the EventTile as a prop so it is not triggering a parse for each tile

t3chguy · 2017-05-18T21:44:03Z

src/components/views/rooms/EventTile.js

        // cosmetic padlocks:
+        if (UserSettingsStore.getSyncedSetting('showTwelveHourTimestamps')) {


take a look at the classnames lib, would allow you to do this all in one line -ish

t3chguy · 2017-05-18T21:46:06Z

src/DateUtils.js

    },

    formatTime: function(date) {
+        if (UserSettingsStore.getSyncedSetting('showTwelveHourTimestamps')) {
+          return twentyfourhour(date);


4 spaces please :)

t3chguy · 2017-05-18T21:46:30Z

src/DateUtils.js

+  let ampm = hours >= 12 ? 'PM' : 'AM';
+  hours = hours % 12;
+  hours = hours ? hours : 12;
+  minutes = minutes < 10 ? '0'+minutes : minutes;


a template literal might be more readable here or at least on the line below

ara4n · 2017-05-18T22:18:58Z

tests are failing - my guess (untested) is that you need mkStubRoom() in test/test-utils.js to return a valid event for getAccountData(), like createTestClient() does. Atm it returns null, hence the NPE. You'll want to run npm run test to see it go up in a ball of flame (and to check that you've fixed the tests)

I'm also slightly having second thoughts about whether EventTile should be using a prop for this (which is set by the room itself) rather than querying the js-sdk from the depths of its render() method. If it was set by the room, then we'd have the advantage of being able to listen for Room.accountData events and update the prop when that happens, thus responding to synced settings changes in realtime rather than waiting for the page to be reloaded...

Pad time

ara4n · 2017-05-26T15:52:07Z

lgtm! sorry for slow review; we're drowning.

Kieran-Gould mentioned this pull request May 18, 2017

Kieran gould/12hourtimestamp element-hq/element-web#3961

Merged

Kieran-Gould changed the base branch from master to develop May 18, 2017 21:24

ara4n reviewed May 18, 2017

View reviewed changes

t3chguy reviewed May 18, 2017

View reviewed changes

ara4n assigned Kieran-Gould May 18, 2017

Kieran-Gould added 4 commits May 19, 2017 22:29

Add 12 hour support

008cc95

Rename twentyFourHour

5aa1bc4

Pad time

Add parameter showTwelveHours to formatTime

f9152b2

Add 12 hour class to mx_EventTile

6b32975

Kieran-Gould force-pushed the kierangould/12hourtimestamp branch from 17bd110 to 6b32975 Compare May 19, 2017 21:36

Moved isTwelveHour alllll the way up to TimelinePanel.

47e5e8d

Kieran-Gould force-pushed the kierangould/12hourtimestamp branch from a069cbf to 47e5e8d Compare May 19, 2017 22:04

ara4n merged commit fde553a into matrix-org:develop May 26, 2017

ara4n mentioned this pull request May 26, 2017

option to show 12h formatted timestamps element-hq/element-web#1728

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kierangould/12hourtimestamp #903

Kierangould/12hourtimestamp #903

Kieran-Gould commented May 18, 2017

matrixbot commented May 18, 2017

ara4n May 18, 2017

ara4n May 18, 2017

ara4n May 18, 2017

ara4n May 18, 2017

t3chguy May 18, 2017 •

edited

Loading

t3chguy May 18, 2017

t3chguy May 18, 2017

t3chguy May 18, 2017

t3chguy May 18, 2017

t3chguy May 18, 2017

ara4n commented May 18, 2017 •

edited

Loading

ara4n commented May 26, 2017

		@@ -16,6 +16,8 @@ limitations under the License.

		'use strict';

		import UserSettingsStore from '../../../UserSettingsStore';

		// cosmetic padlocks:
		if (UserSettingsStore.getSyncedSetting('showTwelveHourTimestamps')) {

Kierangould/12hourtimestamp #903

Kierangould/12hourtimestamp #903

Conversation

Kieran-Gould commented May 18, 2017

matrixbot commented May 18, 2017

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

t3chguy May 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ara4n commented May 18, 2017 • edited Loading

ara4n commented May 26, 2017

t3chguy May 18, 2017 •

edited

Loading

ara4n commented May 18, 2017 •

edited

Loading