-
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): Add start/end workflows ISO display switch #13284
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.
Fixes #12943
I thought that issue was for more than just the start/end times on the node side panel, no?
@agilgur5 I updated my PR to include every Timestamp components corresponding to the timestamps on major views Also I face a UI issue on list so the icon isn't always shown (STARTED vs FINISHED on my screenshot), so what would be the way to handle this in your opinion ? Is it relevant to be able to switch here ? |
@agilgur5 I moved the time switch to the table header to make better experience. This PR is ready to review now. |
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.
Maybe also show what format this will be changed to once it's clicked?
You mean in the tooltip title ? To change |
Yes exactly. Just a suggestion though. Not a blocking comment. |
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
c6fab17
to
f02c44b
Compare
@terrytangyuan I implemented your suggestion, now ready to review |
Thanks. Would you mind sharing the screenshots? |
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.
Perfect. Thank you for your hard work on this!
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
ui/src/app/cluster-workflow-templates/cluster-workflow-template-list.tsx
Show resolved
Hide resolved
@@ -51,6 +52,8 @@ export function ClusterWorkflowTemplateList({history, location}: RouteComponentP | |||
|
|||
useCollectEvent('openedClusterWorkflowTemplateList'); | |||
|
|||
const [storedDisplayISOFormat, setStoredDisplayISOFormat] = useTimestamp(TIMESTAMP_KEYS.CLUSTER_WORKFLOW_TEMPLATE_LIST); |
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.
similarly, the existing style has all const
s at the top of the component, while this is an outlier. please follow the existing style
ui/src/app/workflows/components/workflow-details-list/workflow-details-list.tsx
Show resolved
Hide resolved
ui/src/app/workflows/components/workflow-node-info/workflow-node-info.tsx
Show resolved
Hide resolved
const storage = new ScopedLocalStorage('Timestamp'); | ||
|
||
// key is used to store the preference in local storage | ||
const useTimestamp = (timestampKey: TIMESTAMP_KEYS): [boolean, (value: boolean) => void] => { |
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.
Per #13160 (comment) and #12062 et al, please use named functions instead of consts assigned to anonymous functions.
this file seems to be an outlier in this usage amongst the rest of the code in this PR
const useTimestamp = (timestampKey: TIMESTAMP_KEYS): [boolean, (value: boolean) => void] => { | ||
const [storedDisplayISOFormat, setStoredDisplayISOFormat] = useState<boolean>(storage.getItem(`displayISOFormat-${timestampKey}`, false)); | ||
|
||
const handleStoredDisplayISOFormatChange = (value: 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.
same as above
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.
Sorry that I did not get to this earlier; as you might be able to tell, my list of todos has been overflowing for some time.
It looks like this can be quite heavily simplified with a single global, which would be more consistent and match the "Settings" / "Preferences" page intent of #12943 (comment)
<a> | ||
<i |
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 isn't a button nor has the role of a button, so this will cause accessibility issues and keyboard limitations. Related: argoproj/argo-ui#221
WORKFLOWS_ROW_FINISHED = 'workflowsRowFinished', | ||
CRON_ROW_STARTED = 'cronRowStarted', | ||
CRON_ROW_FINISHED = 'cronRowFinished' | ||
} |
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.
So in #12943 (comment) I had proposed a "Settings" or "Preferences" page. That would also help with width issues from the icon, but the more important part is the functionality itself:
Rather than having a separate state for each page or multiple states on the same page, there would be a single global.
In the approach you have here, you wouldn't need any of these keys and instead they all share the same localStorage
entry / same context
(see also #12986 (comment)).
I think that is more consistent, especially when on the same page or multiple columns of the same page. Those especially having simultaneously different formats feels pretty confusing to me.
On top of that, reducing this to a single global would significantly simplify the usage in other components -- there would no longer be state and callbacks to pass through nor constants to use. (there's some change detection you'd need to handle, but there's a couple ways of doing that)
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 hook is only ever used with the Timestamp
components, so I would suggest placing them in the same file as well (although it may not be necessary to export at all with a global as per above; depends on how you do it).
Same as in #13593 (comment), that would combine the imports to make the usage very self-evident as import {Timestamp, useTimestamp}
, which is also a common convention in the React ecosystem
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]>
Signed-off-by: Adrien Delannoy <[email protected]> Co-authored-by: Adrien Delannoy <[email protected]> Backports: argoproj#13284 Fixes: argoproj#12943
Fixes #12943
Motivation
To give the user the ability to display workflows start/stop time in ISO format.
Modifications
I added the ability to switch time format with a toggle clock icon. This settings is persisted in localStorage.
Verification
I tested on multiple workflows to make sure this settings is shared across workflows.