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

add ability to set a specific timezone for an individual picker #420

Open
dalelotts opened this issue Mar 2, 2018 · 10 comments
Open

add ability to set a specific timezone for an individual picker #420

dalelotts opened this issue Mar 2, 2018 · 10 comments

Comments

@dalelotts
Copy link
Owner

Default to browser timezone if not set.

Timezone is considered in the determination of .now, parsing dates from the model, serializing dates to the model, and raising change events

@gmiller-in-situ
Copy link

I'm looking into working on this because we need it for our application and I'd rather contribute here than write our own adapter.
Is this still relevant?
Is it okay if it only works with moment-timezone, or do we want people to be able to pass in offsets in "(+-)HHmm"?

@dalelotts
Copy link
Owner Author

dalelotts commented Jan 6, 2020

Hey @gmiller-in-situ thanks for looking into this enhancement. I'm not sure how relevant this feature is/will be for the existing user base but if you will find it helpful I'll work with you to get a PR merged.

I think moment-timezone will be fine.

Off the top of my head something like the following seems reasonable.

<dl-date-time-picker
	startView="day"
	maxView="year"
	minView="minute"
	minuteStep="5"
	[(ngModel)]="selectedDate"
	timeZone="America/Los_Angeles"	<-- Standard time zone identifier
>
</dl-date-time-picker>

This would cause the picker to display and store all dates in the America/Los_Angeles timezone regardless of the end-users actual timezone. Of course this input should support binding to a property.

I'm open to using either timezone or offset. It seems that timezone will be more accurate as offset for a timezone can/has/will change over time. 😄

I haven't thought about how the input directive will work when specifying timezone. This enhancement was written before the input directive was added.

The input directive allows for storage in one format, display in a different format, and input in multiple formats so I think that might be a bit more complicated. Would this enhancement support displaying in one timezone, saving the model in a different timezone? Something to think about.

To be clear, these are not requirements or anything, just stuff to think about as you work on your changes,

@gmiller-in-situ
Copy link

gmiller-in-situ commented Jan 8, 2020

I've spent some time looking into this, and there are a number of reasons why I'm going to drop this enhancement.

  • Moment-timezone is a very large library, and getting it as an optional library that can be included only with a specific module import is looking like way more work than seems worthwhile
  • Passing timezones around would cause a lot of refactoring, and it feels like the improvement changes the behavior of the picker in ways which are unexpected and could cause issues for people who are already integrating.

I've written an adapter in our code that can sit on top of this library and apply offsets after users have selected a time. This is fairly simple and doesn't require changing the library's function. The component that the datetime picker is used within can have something like the following to move times emitted from the picker to a desired timezone:

private applyTimezone(date: Moment, timezone?: string, keepLocalTime?: boolean) {
    if (this.useLocalTime) {
      // Apply timezones to the moment
      if (timezone) {
        return moment(date).tz(timezone, keepLocalTime);
      }
      // Default to browser's timezone
      return moment(date).tz(moment.tz.guess(), keepLocalTime);
    } else {
      // Move to UTC
      return moment(date).utc(keepLocalTime);
    }
  }

I still really appreciate having this library, it's a very good tool for the task we use it for!

edit: a code comment

@dalelotts
Copy link
Owner Author

dalelotts commented Jan 8, 2020

I'm glad you find it useful and appreciate you posting your code here. How is the code above called in your project?

@gmiller-in-situ
Copy link

This function would be called on the value emitted by the (change) of the dl-date-time-picker.
The handler function bound to the emitter would look like:

public dateSelected(event: DlDateTimePickerChange<Moment>) {
  value = this.applyTimezone(event.value, 'America/Denver', true);
}

This moves the time from the timezone-unaware space of the date-time-picker to a timezone-aware space in our code.

@dalelotts
Copy link
Owner Author

thanks @gmiller-in-situ - Seems simple enough and I see why there is no need to change this component. I'm glad it all worked out for you.

@dalelotts
Copy link
Owner Author

I have a question @gmiller-in-situ, how does that work when the user opens the picker a second time?

It seems like the selected date/time would be incorrect from the users perspective, especially when the applied timezone would result in a different date - i.e. applying Beijing timezone when the user is in America/Denver. Did you also solve this issue?

@gmiller-in-situ
Copy link

Yeah, it doesn't totally solve the issue. When applying a timezone outside of the datepicker, we're ignoring the [(ngModel)]-bound property and instead relying on the value coming out of the (change). So the property bound to the [(ngModel)] is not modified with a timezone.

@dalelotts
Copy link
Owner Author

Ah, that makes sense. You made me think of something.

Did you consider supplying your own DlDateAdapterMoment that applies / removes the timezone? I'm wondering if this will let you use the [(ngModel)] binding directly.

@gmiller-in-situ
Copy link

Yeah I'm considering coming back to this and trying something where you can just pass the picker a moment with a timezone, and it keeps the timezone intact. I think this would be the best way to handle this probably, but I don't yet know how feasible it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants