Skip to content
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

parts object so we can work around the off by one day bug #4208

Open
dgreene1 opened this issue Aug 17, 2023 · 6 comments
Open

parts object so we can work around the off by one day bug #4208

dgreene1 opened this issue Aug 17, 2023 · 6 comments

Comments

@dgreene1
Copy link

dgreene1 commented Aug 17, 2023

Is your feature request related to a problem? Please describe.
Yes, this would give users a high quality mechanism to get around the 1 day off bug: #1018

Describe the solution you'd like

We we would add a third parameter to onChange which would enable users to produce a LocalDate (or PlainDate as the Temporal spec calls it) without having to do workarounds such as those described in #1018

export interface IDateParts{
    year: number;
    month: number;
    dayOfMonth: number;
    hour: number;
    minutes: number;
    seconds: number;
    /** a string so that you can capture negative numbers */
    offset: string
}

onChange(
        date: WithRange extends false | undefined ? Date | null : [Date | null, Date | null],
        event: React.SyntheticEvent<any> | undefined,
        parts: IDateParts 
    ): void;

For example, you could then use that parts object to do a high quality translation to LocalDate since you'd know that you can disregard hours and minutes:

import { LocalDate } from  "@js-joda/core"

const handleOnChange = (,,parts) => {
  if(parts.hours === null || parts.hours === undefined){
    // We now know that the selection was a day not a date/time
    const myLocalDate= LocalDate.of(parts.year, parts.month, parts.dayOfMonth));
    setDate(myLocalDate);
  }
}

And of course you can still handle moments in time types too:

import { ZonedDateTime } from  "@js-joda/core"

const handleOnChange = (,,parts) => {
  var myZDT = ZonedDateTime.of(
    LocalDate.of(parts.year, parts.month, parts.dayOfMonth),
    LocalTime.of(parts.hour, parts.minutes),
    ZoneId.SYSTEM
  );
  setDate(myZDT);
}

Describe alternatives you've considered
There are a great many workarounds listed in #1018 and none of them are considered to be universally successful or more superior to another. My approach described in https://dev.to/dgreene1/making-your-datepicker-easier-to-work-with-21n4 gets closer by using domain driven types (like the Temporal spec) to give more clarity. But that approach is also hamstrung by having to work on formatting the ISO string. So the approach above removes the JS Date object from the mix entirely and allows true representation of LocalDate (i.e. a date without time).

Additional context

  • The numbers and strings in the parts object would have to have the same index (i.e. 0 or 1) as the JS date numbers and JsJoda's so we would create strong interop and future proof for Temporal.
  • While I would classify this as a workaround at the moment due to the desire to make a non-breaking change, ideally in the future the onChange method would not return a Date object at all since that is inherently the problem.
  • I'm not sure that offset is something we need to support. Any thoughts? If this was approved I would only add offset if we were able to make sure it was the offset that the picker was using.
  • I wish that we didn't have to support seconds since it’s not available in the calendar area (unless Add support for seconds to the timepicker #3214 were to be reopened and merged); however, due to the ability to pass in a custom format, the input allows people to type fractions of seconds:

image

@RHeynsZa
Copy link

Looks clean. Also keep the ability for developers to use the date library of their choice (luxon, datejs, Day.js etc)

@dgreene1
Copy link
Author

@martijnrusschen does the proposal look good to you?

@martijnrusschen
Copy link
Member

Sorry for the delay. Makes sense to me.

@dgreene1
Copy link
Author

dgreene1 commented Oct 9, 2023

Thank you for approving the solution. Unfortunately our team has moved into another focus and will be unable to implement this. However, I do feel that this is winning solution to the associate issue which is multiple years old. So I hope that someone will choose to implement it.

Copy link

This issue is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label May 30, 2024
@dgreene1
Copy link
Author

I’m requesting that this stay open (ie remove the stale label) since the proposed solution has the potential to close the multi year issue that is plaguing many business.

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

No branches or pull requests

3 participants