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

Drop datetime_picker_rails and use browser fields #2136

Merged
merged 1 commit into from
Feb 11, 2022

Conversation

nickcharlton
Copy link
Member

Since Administrate was released, browser support for native fields
around date/time/datetime picking has improved substantially. Now, every
major browser has adequate support for native pickers.

For those who need to support older browsers, we'd recommend using a
polyfill such as: https://github.com/jonstipe/datetime-local-polyfill

This commit should maintain like-for-like compatability, including
allowing filling out seconds by setting the step attribute.

https://caniuse.com/input-datetime
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/datetime-local
https://stackoverflow.com/questions/20111413/html5-datetime-local-control-how-to-hide-seconds

Since Administrate was released, browser support for native fields
around date/time/datetime picking has improved substantially. Now, every
major browser has adequate support for native pickers.

For those who need to support older browsers, we'd recommend using a
polyfill such as: https://github.com/jonstipe/datetime-local-polyfill

This commit should maintain like-for-like compatability, including
allowing filling out seconds by setting the `step` attribute.

https://caniuse.com/input-datetime
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/datetime-local
https://stackoverflow.com/questions/20111413/html5-datetime-local-control-how-to-hide-seconds
@nickcharlton nickcharlton force-pushed the nc-use-native-datetime-picker branch from ef4e08a to a07b1d5 Compare February 4, 2022 10:52
@pablobm
Copy link
Collaborator

pablobm commented Feb 10, 2022

Argh. Firefox ESR is v91 at the moment, while compatibility with datetime-local appears to have come in at v93. This actually affects me, as I run Debian on my Open Source environment and of course I'm years behind the curve...

My selfish use case aside, I wonder if keeping compatibility with ESR versions of major browsers would be sensible. Also, caniuse gives a global compatibility of 81%, which is not that great. From what I can gather, the next Firefox ESR is slated for 2022-06-28 (v102), so I think it's not that far away in relative terms. I can't tell what the situation is with Chrome or Safari though; info is not clear for them.

But I can be swayed either way 🙂 Thoughts?

@pablobm
Copy link
Collaborator

pablobm commented Feb 10, 2022

Another possible option: we include the polyfill (vendored) by default, and remove it later.

@nickcharlton
Copy link
Member Author

nickcharlton commented Feb 10, 2022

Oh, dammit. I didn't consider Firefox ESR, which is funny as I have a similar personal setup (Debian + Firefox).

I'm disappointed how slow the rollout of datetime-local has been, as I've wanted to remove this dependency for a long time (as the first in a few which is making things difficult).

My feeling is that in the case of IE, Firefox ESR, etc is ideal for a polyfill, especially as for the latter it's a relatively short-term problem.

@pablobm
Copy link
Collaborator

pablobm commented Feb 10, 2022

I'm less worried about IE11, partially because I'm a bad person 😬 and partially because I feel that the industry has moved on. And yet... hm, more options:

  • We offer some really simple way to turn the polyfill on/off. Eg: a generator and a question on the install generator, which copy the polyfill to a vendored location, and this is only loaded if it's present at the location.
  • We change our Date, Time and DateTime fields to use type=time and type=date. This bring us down to Firefox 57 (if I'm reading this correctly), but we'd still need a polyfill for IE.

I might be making this more complex than it needs to be...

@nickcharlton
Copy link
Member Author

Yeah, same, agreed on the IE problem. I sort of forget how much time has moved on there and we're okay to ignore it.

I did notice time/date did work, and that might be a good option. I do feel like all of these are quite a bit of work, and by the time we do a release with these in, Firefox ESR might be updated so that it's no longer a problem.

@pablobm
Copy link
Collaborator

pablobm commented Feb 10, 2022

Good point on the rework vs time: it's not worth it.

I say, let's vendor the polyfill. That solves the problem now. We can remove it in the future, perhaps in a staggered manner (providing an easy option or something), or perhaps we just remove it and not worry too much.

Hypothesis (totally out of my... sleeve): Administrate is being used in environments where the admins/devs know who the audience is going to be. They can use documentation/options to enable the polyfill in the future (after we remove the default) if needs be.

@nickcharlton
Copy link
Member Author

I'm going to take the risky position and remove it and not worry too much. I'm of the same feeling that users of Administrate are in a position to know the browsers their users are using and holding back because Firefox ESR, in a release where I'm expecting we'll make a lot of other breaking asset changes is a painful step we can't avoid.

I do think it's going to be really important to put together a detailed upgrade guide, and explain how to add the polyfill for users who will need it though, otherwise this is going to be really, really annoying for people.

@nickcharlton nickcharlton merged commit 5958b54 into main Feb 11, 2022
@nickcharlton nickcharlton deleted the nc-use-native-datetime-picker branch February 11, 2022 11:59
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.

2 participants