Skip to content

Conversation

@meghanmae
Copy link
Contributor

Description

  • added a 'today' button on the date time picker.
  • I can revert, but I also enabled the adjacent months to reduce the amount of whitespace

Questions:

  • Do you want a test written for this? I made a small attempt, but ran into some issues. Can try again if a test would be valuable.
  • Are these too close together?
    image

Ensure that your pull request has followed all the steps below:

  • Code compilation
  • Created tests which fail without the change (if possible)
  • All tests passing
  • Extended the README / documentation, if necessary

@meghanmae meghanmae changed the base branch from dev to next October 25, 2024 21:47
@ascott18
Copy link
Collaborator

next isn't currently active

@ascott18 ascott18 changed the base branch from next to dev October 25, 2024 23:05
v-bind="datePickerProps"
>
<template v-slot:actions v-if="showTodayButton">
<v-btn @click="setToday" :disabled="!isDateAllowed(new Date())"> Today </v-btn>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checking if today's date is allowed... is just passing 'new Date()' here sufficient? Is there any worry about time zones?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would certainly be easier to not do this. Leave it up to the developer to just not enable the "Today" button if they know for their use case that selecting today will be out of bounds of their validation rules.

This validation is happening before the timezone transformations has been applied, so yes, there will be timezone issues here. The other usage of isDateAllowed in emitInput occurs after timezone transformations.

@meghanmae meghanmae requested a review from ascott18 December 27, 2024 16:40
<v-text-field
v-else
class="c-datetime-picker"
:class="{ 'today-btn': showTodayButton }"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an unusual class name. The entire component is not a today-btn. Only the button itself inside the component's menu would make sense to have a today-btn class.

Consider a more appropriate class name like c-datetime-picker__menu--has-today-btn on the menu's contentClass, which also adheres to the (mildly consistent) use of BEM selectors in this project.

@meghanmae meghanmae force-pushed the gravatt/next/today-btn branch from 3f3c313 to ae5e482 Compare February 28, 2025 16:44
}
}
function isDateAllowed(date: Date) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should just be axed and if you click 'today' when there is a min/max value and today doesn't fall within it we just clamp to be within the bounds?

@meghanmae meghanmae force-pushed the gravatt/next/today-btn branch from f14ec98 to 7375921 Compare February 28, 2025 17:00
@ascott18 ascott18 changed the base branch from dev to next April 1, 2025 16:42
@ascott18 ascott18 merged commit 38ee538 into next Apr 1, 2025
7 checks passed
@ascott18 ascott18 deleted the gravatt/next/today-btn branch April 1, 2025 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants