Add two-line timestamp and remove dayjs #1333

rogermparent · 2022-02-17T00:50:32Z

This PR changes the experiment table's timestamp format to have two lines, one prominent one with the time and a smaller sub-line below it with the date. We've talked in meetings about using a consistent timestamp between products, but from looking at those products (DVC CLI and Studio) it seems like most use the date format we've had previously so whatever we go with will need to be something new.

The formatting is also changed to use native JS Date and its toLocaleDate and toLocaleTime methods. Since this removes the last remaining usage of dayjs, this PR also removes the library. The logic of using different formats depending on whether the date is today or before is also removed, and all dates are now displayed consistently regardless of their position relative to the current day.

I'm absolutely open to any criticism and/or requested changes, this is just a shot in the dark from what I assume would be a good way to display all the necessary information.

Newest demo:

new-timestamp-demo.mp4

Old demo:

new-timestamp-demo.mp4

Fixes #1270

rogermparent · 2022-02-17T00:51:22Z

webview/src/experiments/components/Table/styles.module.scss

+  overflow: hidden;
+  text-overflow: ellipsis;


This fixes ellipsis truncation in the nested timestamp divs

rogermparent · 2022-02-17T00:51:51Z

webview/src/experiments/components/Table/styles.module.scss

@@ -519,6 +521,7 @@ $spinner-color-light: #000;
  padding: 0 $cell-padding;
  align-items: center;
  width: 100%;
+  height: 100%;


This fixes vertical centering in cells whose contents aren't full height via line-height

mattseddon

Looks good, some thoughts:

Cli output time is 0 padded:

Left/right truncation of the Timestamp header/column looks odd (especially next to Experiments header):

mattseddon · 2022-02-17T01:47:23Z

For 2. probably makes more sense to change the header.

rogermparent · 2022-02-17T03:10:43Z

webview/src/experiments/components/Experiments/index.tsx

+const timeFormatter = new Intl.DateTimeFormat([], {
+  hour: '2-digit',
+  minute: '2-digit'
+})
+const dateFormatter = new Intl.DateTimeFormat([], {
+  dateStyle: 'medium'
+})
+


When formatting large numbers of dates, it is better to create an Intl.DateTimeFormat object and use the function provided by its format property.

rogermparent · 2022-02-17T03:11:13Z

webview/src/experiments/components/Experiments/index.tsx

@@ -73,16 +80,21 @@ const getColumns = (columns: MetricOrParam[]): Column<Experiment>[] =>
        if (!value || value === '') {
          return null
        }
-        const time = dayjs(value)
+        const date = new Date(value)


Might be better to push this further up into data parsing, but this will do for now

Yep definitely move it in case we need to use this timestamp somewhere else. We should also standardise all dates in the extension to look the same.

Can follow up.

I can do the follow-up for pushing new Date up into webview parsing, I'm not sure where else we display date timestamps though.

rogermparent · 2022-02-17T03:12:37Z

webview/src/experiments/components/Table/TableHeader.tsx

-          <div className={styles.headerCellContentsWrapper}>
-            <span
-              ref={provided.innerRef}
-              {...provided.draggableProps}
-              {...provided.dragHandleProps}
-              data-testid="rendered-header"
-              style={provided.draggableProps.style}
-              className={cx(styles.cellContents, {
-                [styles.draggingColumn]: snapshot.isDragging,
-                [styles.staticColumn]: !snapshot.isDragging,
-                [styles.isDroppedColumn]: snapshot.isDropAnimating
-              })}
-            >
-              {column.render('Header')}
-            </span>
-          </div>
+          <span
+            ref={provided.innerRef}
+            {...provided.draggableProps}
+            {...provided.dragHandleProps}
+            data-testid="rendered-header"
+            style={provided.draggableProps.style}
+            className={cx(styles.cellContents, {
+              [styles.draggingColumn]: snapshot.isDragging,
+              [styles.staticColumn]: !snapshot.isDragging,
+              [styles.isDroppedColumn]: snapshot.isDropAnimating
+            })}
+          >
+            {column.render('Header')}
+          </span>


headerCellContentWrapper is pushed into header render functions so it can be left off of Timestamp to enable selective rtl/ltr truncation.

rogermparent · 2022-02-17T03:13:23Z

webview/src/experiments/util/buildDynamicColumns.tsx

+  return (
+    <div className={styles.headerCellContentsWrapper}>
+      <span title={name}>{name}</span>
+    </div>
+  )


headerCellContentsWrapper pushed here to apply to all normal headers.

rogermparent · 2022-02-17T03:19:43Z

Cli output time is 0 padded:

Left/right truncation of the Timestamp header/column looks odd (especially next to Experiments header):

For 2. probably makes more sense to change the header.

@mattseddon Good points! I did 1 with a slight tweak to time formatter options and 2 with a slightly larger refactor. On the same line of thought as matching CLI, I changed the date formatting to MMM, DD, YYYY like CLI has:

sroy3 · 2022-02-17T15:37:14Z

Screen.Recording.2022-02-17.at.10.36.02.AM.mov

From the UI tests, seems like the header of experiment and timestamp aren't aligned vertically anymore.

codeclimate · 2022-02-17T19:00:49Z

Code Climate has analyzed commit 6e78980 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (85% is the threshold).

This pull request will bring the total coverage in the repository to 96.2% (0.0% change).

View more on Code Climate.

rogermparent · 2022-02-17T19:15:05Z

I re-added the same padding that normal headers have to the Experiment and Timestamp cells, then grabbed a screenshot from the latest Chromatic build to ensure that Timestamp and Experiment cells are now at the same vertical alignment:

There's still a diff highlight, I guess because the baseline got updated to the previous commit, but from that check I'd say the issue's been solved. Thanks for pointing it out @sroy3!

rogermparent · 2022-02-17T19:22:43Z

Accepted the latest Chromatic diff myself and will merge, still open to any further changes in follow-ups!

Add two-line timestamp and remove dayjs

bb72a96

rogermparent requested review from mattseddon and sroy3 February 17, 2022 00:50

rogermparent self-assigned this Feb 17, 2022

rogermparent added the product PR that affects product label Feb 17, 2022

rogermparent commented Feb 17, 2022

View reviewed changes

mattseddon approved these changes Feb 17, 2022

View reviewed changes

rogermparent added 2 commits February 16, 2022 22:02

Change date format and right-truncate timestamp header

43935e1

right-align timestamp cell

971bb05

rogermparent commented Feb 17, 2022

View reviewed changes

rogermparent requested a review from shcheklein February 17, 2022 03:28

sroy3 approved these changes Feb 17, 2022

View reviewed changes

rogermparent added 2 commits February 17, 2022 13:42

Re-apply vertical padding to experiment and timestamp headers

2773038

Merge branch 'master' into new-timestamp

6e78980

rogermparent merged commit 9a53621 into master Feb 17, 2022

rogermparent deleted the new-timestamp branch February 17, 2022 19:22

rogermparent mentioned this pull request Mar 14, 2022

Improve how timestamp column formats times #1444

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add two-line timestamp and remove dayjs #1333

Add two-line timestamp and remove dayjs #1333

rogermparent commented Feb 17, 2022 •

edited

Loading

rogermparent Feb 17, 2022

rogermparent Feb 17, 2022

mattseddon left a comment

mattseddon commented Feb 17, 2022

rogermparent Feb 17, 2022

rogermparent Feb 17, 2022

mattseddon Feb 17, 2022

mattseddon Feb 17, 2022

rogermparent Feb 17, 2022

rogermparent Feb 17, 2022

rogermparent Feb 17, 2022 •

edited

Loading

rogermparent commented Feb 17, 2022 •

edited

Loading

sroy3 commented Feb 17, 2022

codeclimate bot commented Feb 17, 2022

rogermparent commented Feb 17, 2022

rogermparent commented Feb 17, 2022

Add two-line timestamp and remove dayjs #1333

Add two-line timestamp and remove dayjs #1333

Conversation

rogermparent commented Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattseddon left a comment

Choose a reason for hiding this comment

mattseddon commented Feb 17, 2022

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

rogermparent Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

rogermparent commented Feb 17, 2022 • edited Loading

sroy3 commented Feb 17, 2022

codeclimate bot commented Feb 17, 2022

rogermparent commented Feb 17, 2022

rogermparent commented Feb 17, 2022

rogermparent commented Feb 17, 2022 •

edited

Loading

rogermparent Feb 17, 2022 •

edited

Loading

rogermparent commented Feb 17, 2022 •

edited

Loading