NXT-4230: Make datetime format consistent with version panel on hub#115
NXT-4230: Make datetime format consistent with version panel on hub#115hriverahdez merged 3 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Updates how “created on” timestamps are rendered in the Change Hub Item Version modal to match the datetime formatting used elsewhere (hub version panel).
Changes:
- Added a
formatCreatedOnhelper to produce a consistent “Created on {date}, {time}” label. - Updated dropdown option construction to use the new helper instead of the prior “Created: {date}” formatting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
org.knime.ui.js/src/components/workflowEditor/ChangeHubItemVersionModal.vue
Outdated
Show resolved
Hide resolved
org.knime.ui.js/src/components/workflowEditor/ChangeHubItemVersionModal.vue
Outdated
Show resolved
Hide resolved
org.knime.ui.js/src/components/workflowEditor/ChangeHubItemVersionModal.vue
Outdated
Show resolved
Hide resolved
xnhp
force-pushed
the
enh/NXT-4230-webui-for
branch
from
7785db5 to
e8cfb85
Compare
|
|
||
| const MAX_DESCRIPTION_LENGTH = 120; | ||
|
|
||
| const formatCreatedOn = (createdOnValue: string | Date) => { |
There was a problem hiding this comment.
I have many questions 😄
- When is this a Date object? If it comes from the BE it's always going to be transferred serialized as a string and has to be converted manually
- The date validation is incorrect (see img)
- Why format the time for a locale, but not the date?
- For formatting dates/times it's better to use this https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl
- Otherwise, if we should use one singular format, I'd suggest something like this https://vueuse.org/shared/useDateFormat/#usedateformat
bottom line: dates are awkward in JS, don't understimate them
There was a problem hiding this comment.
In the generated gateway API, NamedItemVersion.createdOn is in fact declared as a Date.
But apparently it can be declared a Date but still actually be a String at 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?
There was a problem hiding this comment.
But apparently it can be declared a Date but still actually be a String at runtime since (is that correct?) typescript types really "exist" only at compile time.
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
The locale specification was intentional to be consistent with hub webapp
Ah, I see
We can already start defining one singular format for the frontend but not sure if its worth it right now.
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.
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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 dateLabel = formatDateString(date.toISOString()); | ||
| const timeLabel = new Intl.DateTimeFormat([], { | ||
| hour: "numeric", | ||
| minute: "2-digit", | ||
| }).format(date); | ||
| return `Created on ${dateLabel}, ${timeLabel}`; | ||
| }; |
There was a problem hiding this comment.
This mixes UTC and local time: dateLabel is derived from date.toISOString() (UTC-based), while timeLabel is formatted from the Date object in the user’s local timezone. Around midnight, this can produce inconsistent labels like a UTC date with a local time from the previous/next day. Use a single timezone basis for both (e.g., format both parts via the same Intl.DateTimeFormat with an explicit timeZone, or ensure formatDateString formats from the same Date/timezone used for the time).
org.knime.ui.js/src/components/workflowEditor/ChangeHubItemVersionModal.vue
Outdated
Show resolved
Hide resolved
xnhp
force-pushed
the
enh/NXT-4230-webui-for
branch
from
2c925c5 to
82b92f0
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
2 participants
NXT-4230 (WebUI for "Change Component Link Version")