-
Notifications
You must be signed in to change notification settings - Fork 0
NXT-4230: Make datetime format consistent with version panel on hub #115
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,7 +7,7 @@ import { | |
| KdsModal, | ||
| KdsRadioButtonGroup, | ||
| } from "@knime/kds-components"; | ||
| import { formatDateString } from "@knime/utils"; | ||
| import { formatDateTimeString } from "@knime/utils"; | ||
|
|
||
| import { | ||
| ItemVersion, | ||
|
|
@@ -67,6 +67,19 @@ type DropdownOption = { | |
|
|
||
| const MAX_DESCRIPTION_LENGTH = 120; | ||
|
|
||
| const formatCreatedOn = (createdOnValue: string | Date) => { | ||
| const date = | ||
| createdOnValue instanceof Date ? createdOnValue : new Date(createdOnValue); | ||
| if (Number.isNaN(date.getTime())) { | ||
| const rawValue = | ||
| createdOnValue instanceof Date ? null : createdOnValue.trim(); | ||
| return rawValue ? `Created on ${rawValue}` : "Created on unknown date"; | ||
| } | ||
| const dateInput = | ||
| createdOnValue instanceof Date ? date.getTime() : createdOnValue; | ||
| return `Created on ${formatDateTimeString(dateInput)}`; | ||
| }; | ||
|
Comment on lines
70
to
81
|
||
|
|
||
| const dropdownOptions = computed<DropdownOption[]>(() => | ||
| itemVersions.value | ||
| .filter( | ||
|
|
@@ -77,13 +90,7 @@ const dropdownOptions = computed<DropdownOption[]>(() => | |
| const title = version.title ?? "Untitled"; | ||
| const author = version.author ? `Author: ${version.author}` : null; | ||
| const createdOnValue = version.createdOn; | ||
| const createdOn = createdOnValue | ||
| ? `Created: ${formatDateString( | ||
| createdOnValue instanceof Date | ||
| ? createdOnValue.toISOString() | ||
| : createdOnValue, | ||
| )}` | ||
| : null; | ||
| const createdOn = createdOnValue ? formatCreatedOn(createdOnValue) : null; | ||
| const rawDescription = version.description?.trim() ?? ""; | ||
| let description: string | null = null; | ||
| if (rawDescription.length > 0) { | ||
|
|
||
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.
I have many questions 😄
bottom line: dates are awkward in JS, don't understimate them
Uh oh!
There was an error while loading. Please reload this page.
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.
In the generated gateway API,
NamedItemVersion.createdOnis in fact declared as aDate.But apparently it can be declared a
Datebut still actually be aStringat runtime since (is that correct?) typescript types really "exist" only at compile time.The locale specification was intentional to be consistent with hub webapp but I do agree that it is awkward.
I made some improvements to the method, is that better?
We can already start defining one singular format for the frontend but not sure if its worth it right now. What do you think?
Uh oh!
There was an error while loading. Please reload this page.
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.
The type is mapped as Date, but this is just a type. At runtime, the value is transfered over RPC as a date string, so this will never be a Date object
Ah, I see
Would be a good idea. I can bring it up and we can (later) add it to WAC
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.
Oh, actually, there's something already in WAC
https://github.com/knime/webapps-common/blob/master/packages/utils/src/format.ts#L10
Maybe you should use that. It seems to be doing the same and to the same format