Skip to content

feat(functions): Provide formatDateTime function #6543

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

Open
wants to merge 2 commits into
base: stable8
Choose a base branch
from

Conversation

susnux
Copy link
Contributor

@susnux susnux commented Feb 18, 2025

☑️ Resolves

Split the useFormatDateTime composable into a general function without reactivity and the composable that uses it.
This allows to also use this logic in other places.

🖼️ Screenshots

No visual changes.

🏁 Checklist

  • ⛑️ Tests are included or are not applicable
  • 📘 Component documentation has been extended, updated or is not applicable
  • 3️⃣ Backport to next requested with a Vue 3 upgrade

Basically a non-reactive way of `useFormatDateTime`

Signed-off-by: Ferdinand Thiessen <[email protected]>
@susnux susnux added enhancement New feature or request 3. to review Waiting for reviews labels Feb 18, 2025
@susnux susnux added this to the 8.24.0 milestone Feb 18, 2025
@susnux
Copy link
Contributor Author

susnux commented Feb 18, 2025

/backport to next

@susnux susnux added the feature: functions composables, functions, mixins and other non-components label Feb 18, 2025
@ShGKme
Copy link
Contributor

ShGKme commented Feb 25, 2025

Interface seems confusing to me.

  • Function name says formatDateTime, so I'd expect it to format timestamp like 01 Jan 2025 or 12 minutes
  • Documentation says Format as relative time, like Intl.RelativeTimeFormat
  • Composable returns
    • formattedTime which sounds like DateTimeFormat but actually RelativeTimeFormat
    • formattedFullTime which sounds like it is a full form (and there is a short form?) but actually "format (date) time" using DateTimeFormat

What about:

  1. Keeping naming close to native naming and popular libraries:
    • formatTime/formatDateTime to format as date or duration like DateTimeFormat.format()
    • formatRelativeTime to format as relative time like RelativeTime.format()
  2. Doing the same in the composable result
    • formattedTime -> formattedRelativeTime
    • formattedFullTime -> formattedTime/formattedDateTime
  3. Separating a single universal function/composable into 2 with different roles:
    • To keep the interface simpler (less params)
    • To avoid unneeded actions
    • To also have a function for formatted time (that sets the locale)
    • It seems a very rare case when you need both formatted for one date
    • In this case composables can directly return the result without wrapping into an object and be simpler to use
  4. Later adding formatDuration function
  • Also as a new function, not a third way to use a composable
/**
 * Current
 */

// Complex renaming
const { formattedFullTime: formattedBirthday } = useFormatDateTime(birthdayDate)
// Confusing "fulltime" for a duration
const { formattedFullTime: formattedEventDuraction } = useFormatDateTime(eventDuration)
// Not obious that formatted time is relative
const { formattedTime: clearAtLabel } = useFormatDateTime(userStatus.clearAt)

/**
 * Proposal
 */
const formattedBirthday = useFormatDateTime(birthdayDate)
const formattedEventDuraction = useFormatDateTime(eventDuration)
const clearAtLabel = useRelativeTime(userStatus.clearAt)

@susnux
Copy link
Contributor Author

susnux commented Mar 1, 2025

@ShGKme I agree with your points, especially for the functions.
But for the composable I am not sure as one of the use cases is to have relative vs "plain" time as a configurable options, so with your way you need to include two variables and switch between in a computed?

Either way its unrelated to this PR, as it is only about functions and those I agree with and will adjust :)

@ShGKme
Copy link
Contributor

ShGKme commented Mar 1, 2025

But for the composable I am not sure as one of the use cases is to have relative vs "plain" time as a configurable options, so with your way you need to include two variables and switch between in a computed?

Do you mean, to have a switch from relative to absolute/duration value?

Do you have an example for a use case? It seems to me, relative and general time are always different parts of UI

Comment on lines +97 to +102
// Start the interval for setting the current time if relative time is enabled
onMounted(() => {
if (wrappedOptions.value.relativeTime !== false) {
intervalId = window.setInterval(() => { currentTime.value = Date.now() }, updateInterval.value)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't require component to be in the DOM (or even to be a component at all)

Suggested change
// Start the interval for setting the current time if relative time is enabled
onMounted(() => {
if (wrappedOptions.value.relativeTime !== false) {
intervalId = window.setInterval(() => { currentTime.value = Date.now() }, updateInterval.value)
}
})
// Start the interval for setting the current time if relative time is enabled
if (wrappedOptions.value.relativeTime !== false) {
intervalId = window.setInterval(() => { currentTime.value = Date.now() }, updateInterval.value)
}

Comment on lines +104 to +107
// ensure interval is cleared
onUnmounted(() => {
window.clearInterval(intervalId)
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// ensure interval is cleared
onUnmounted(() => {
window.clearInterval(intervalId)
})
// ensure interval is cleared
onScopeDispose(() => {
if (intervalId !== undefined) {
window.clearInterval(intervalId)
}
})

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it is not available in Vue 2

@susnux
Copy link
Contributor Author

susnux commented Mar 1, 2025

Do you have an example for a use case? It seems to me, relative and general time are always different parts of UI

Logreader if the user selects relative time in the settings or "normal time".

@ShGKme ShGKme modified the milestones: 8.24.0, 8.25.0 Apr 2, 2025
@susnux susnux modified the milestones: 8.25.0, 8.26.0 Apr 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews backport-request enhancement New feature or request feature: functions composables, functions, mixins and other non-components
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants