-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(ui): allow to display date in absolute format #12986
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fill out the PR template. The "Motivation" section is particularly relevant here.
DCO is also missing from your commit.
There's also some bugs in the current code and this should apply on a much broader scale if implemented, see below in-line comments
@@ -13,7 +13,7 @@ export function Timestamp({date}: {date: Date | string | number}) { | |||
'-' | |||
) : ( | |||
<span title={tooltip(date)}> | |||
<Ticker intervalMs={1000}>{() => ago(new Date(date))}</Ticker> | |||
<Ticker intervalMs={1000}>{() => (displayFullDate ? new Date(date).toISOString() : ago(new Date(date)))}</Ticker> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This displays how long ago a date was, not the date itself. If anything you'd want to do this conversion on the date after ago
@@ -3,7 +3,7 @@ import * as React from 'react'; | |||
|
|||
import {ago} from '../duration'; | |||
|
|||
export function Timestamp({date}: {date: Date | string | number}) { | |||
export function Timestamp({date, displayFullDate = false}: {date: Date | string | number; displayFullDate?: boolean}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be passed through context, as it should be a global, not a prop passed through many components
@@ -308,6 +310,16 @@ export function WorkflowsList({match, location, history}: RouteComponentProps<an | |||
); | |||
})} | |||
</div> | |||
<label htmlFor='date_format_checkbox'>Show absolute date</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should this be isolated to the WorkflowsList page? There are other pages with dates.
In #12943 (comment), I suggested in a global settings page, and also storing that in localStorage
, not just temporary state.
@KillianHmyd - are you going to continue developing this PR? We've got a customer asking for it. If you don't have time to work through it, would you mind if we contributed the PR and had you do a review? |
Thanks @KillianHmyd - I really appreciate it |
This PR has been automatically marked as stale because it has not had recent activity and needs further changes. It will be closed if no further activity occurs. |
This PR has been closed due to inactivity and lack of changes. If you would like to still work on this PR, please address the review comments and re-open. |
This was superseded by #13284 |
Fixes #TODO
Motivation
Modifications
Verification