Skip to content
This repository has been archived by the owner on Feb 16, 2023. It is now read-only.

Improve date input #177

Closed
wants to merge 8 commits into from
Closed

Improve date input #177

wants to merge 8 commits into from

Conversation

shamoon
Copy link
Contributor

@shamoon shamoon commented Dec 23, 2020

Hey so I know we had issues with using the ngbDatepicker but unfortunately browser support for the plain HTML5 <input type="date"> is abysmal, see: http://www.iwanttouse.com/#input-datetime / https://caniuse.com/input-datetime. In my case, my primary browser is Safari on macOS and the date is just a plain text field which makes editing it even with keyboard entry very slow and unpleasant. This was frustrating me on the document detail page which is why I started this but then I decided to include the filter editor drop downs as well. For the dropdown I discovered some usability issues for example, since theres (currently, anyway) no form validation you can type in anything in safari which results in malformed API requests.

So, there were a few other options I looked at but what I ended up settling on was using a masking package ngx-mask and bringing back the NgbDatePicker (this time with no hacks or issues 🙂). I kinda think its the "best of both worlds".

This PR includes:

  • Prevent non-numeric character input and ensure well-formed dates YYYY-MM-DD
  • Bootstrap date-picker
  • Consistent functionality across all browsers

Of course, welcome your feedback.

Edit: video

p-ng_date.mov

@jonaswinkler
Copy link
Owner

jonaswinkler commented Dec 23, 2020

Thank you! (Edited)

I wasn't aware that support for type="date" inputs is this bad. I've now got a browser installed that does not have support for this (not safari though), and its pretty bad. I assumed that the date picker I was seeing was some bootstrap thing, but apparently its not.

Do you happen to know how we display and edit dates in the currently active locale? (which is en-US right now, but I'll definitely add support for more locales in the future) Such as "mm/dd/yyyy" or "dd. mm. yyyy". This is what browsers with support for type="date" will do automatically.

That has been requested by some users an I want to get that done together with localization. That doesn't seem to be so easy with predefined masks, sadly.

@shamoon
Copy link
Contributor Author

shamoon commented Dec 23, 2020

Yea localization stuff #123 occurred to me while writing this. I think I can sort it out...

@jonaswinkler
Copy link
Owner

Maybe the stuff over here is useful for this.

https://ng-bootstrap.github.io/#/components/datepicker/overview#date-model

@shamoon
Copy link
Contributor Author

shamoon commented Dec 24, 2020

Oof, yea this got ugly fast. I mean it works but Im not happy with it. I made it a separate branch but its really doing my head in. https://github.com/shamoon/paperless-ng/tree/feature-date-picker-i18n

The date picker is trivial to change it just works once you set the locale of the app ( {provide: LOCALE_ID, useValue: 'en-US' } ) but then interpolating all of the dates between components string vs Date is not fun. If youre curious you can look at and try out that branch, otherwise I think options are:

  • We dont update the input field at all for i18n, just leave it as yyyy-MM-dd and at least the date picker will be in the appropriate language.
  • Remove the mask, this way we could actually allow the locale date format in the field so 'en-US' would show M/d/y.
  • Use the above branch (I am sure there are bugs but could perhaps be salvaged)
  • Any other ideas?

@shamoon
Copy link
Contributor Author

shamoon commented Dec 25, 2020

I know its a holiday, sorry for all the messages but please take your time to reply =)

Just wanted to add that I think the above branch ( https://github.com/shamoon/paperless-ng/tree/feature-date-picker-i18n ) isnt actually so bad, just perhaps could use your feedback on the approach. To explain a little more, there are three basic places we would need to internationalize to make this new component work:

  1. The placeholder e.g. M/d/yy in en-US (default) vs. d/M/yy for ca-FR. Placeholder is important to show the user what to enter if using keyboard. This just comes straight from getLocaleDateTimeFormat, easy. The catch is that I think its confusing to show a user "d" but allow them to enter 2 digits (and the mask needs to match to allow X digits), so I transform this i.e. d --> dd.
  2. The mask which essentially needs to follow the above. Again, not so bad and we just use the placeholder, dd from above --> d0 (see https://github.com/JsDaddy/ngx-mask#date-validation ).
  3. Finally there's the actual date that gets entered, which will be entered in its locale format, meaning it will need to be transformed back into something standardized (I used Date) to pass it to the datepicker and for when the date needs to be passed to the API in YYYY-MM-dd format. Essentially this will "undo" calls to formatDate, hence I made a DateDeformatPipe =) . Ideally there would be a more standard way to do this, not sure?

In an effort to keep things DRY I put the above in Pipes, which is probably not the right approach for all 3 but at least as a proof of concept it works. See https://github.com/shamoon/paperless-ng/blob/feature-date-picker-i18n/src-ui/src/app/components/common/input/date-time/date-time.component.ts for reference.
In the end this does work but welcome your feedback. Sorry for the long comment--merry x-mas!

@shamoon shamoon marked this pull request as draft January 18, 2021 03:42
@shamoon
Copy link
Contributor Author

shamoon commented Jan 18, 2021

Hey @jonaswinkler I know this has been bumped for a while but I was circling back around to it and I think with the date formatting in the app now this is worth looking at again. I merged the dev branch with what I had done in terms of localization. So the fact still remains with this new date picker + mask component we have to localize the placeholder, the mask & the date but its not actually so bad and this currently works.

Take a look and tell me what you think. One issue with the code as it is now is I really didn't know the best place to put the functions for getting the formatted mask and placeholder as well as one function that "de-transforms" a localized string, in other words if someone types 13.01.21 with de date format converts that back to a date. So all of those are currently in custom-date.pipe.ts, I realize that may not be the right place but I had them as separate pipes which also seemed wrong (especially because I dont need them as pipes i.e. in templates).

@jonaswinkler
Copy link
Owner

jonaswinkler commented Feb 24, 2021

Finally looking into this again.

  • The pipes are only used for formatting. These are not supposed to provide a reverse function.
  • The date selector of ng-bootstrap has these handy NgbDateAdapter and NgbDateParserFormatter classes, which tell the date selectors how to read/write a NgbDateStruct to a model field, and how to display/parse and NgbDateStruct in the input field. Probably need to write some custom regular expressions for each language, but overall this is not too bad.

image

  • month off by one, not sure about that
  • Date selected is always midnight in the current time zone, so the zulu time string seems off, but it actually is not, considering GMT+1 (which is my time zone)

@shamoon
Copy link
Contributor Author

shamoon commented Feb 24, 2021

Yea it’s been a while so I’d have to dig in again. I think the issue was building an adapter between the mask and date input.

@shamoon
Copy link
Contributor Author

shamoon commented Feb 24, 2021

Ps. Thanks for circling back to stuff, I do realize you’ve been very busy with squashing bugs and structural changes (not to mention, ya know, life!)

jonaswinkler added a commit that referenced this pull request Feb 24, 2021
@jonaswinkler
Copy link
Owner

Well...

Ps. Thanks for circling back to stuff, I do realize you’ve been very busy with squashing bugs and structural changes (not to mention, ya know, life!)

This issue here has been on my list for very long, I just didn't had the motivation to dig into this ng-bootstrap date picker.

I think I've got this sorted out, so I'll be closing this. Regarding masking: not sure if this is exactly needed. The backend will reject invalid dates and the front end will show an error on invalid input.

@shamoon
Copy link
Contributor Author

shamoon commented Feb 24, 2021

Eh, ok your call of course. IMHO errors after submitting are not as good as just making sure the input is correct. Not all browsers will validate and keyboard input is basically unrestricted. Which is why I was trying to implement masking.

But yea...

@jonaswinkler
Copy link
Owner

Oh, my bad. Should have phrased that a little better.

I was not implying that it's not useful! I just that the NgbDateParserFormatter (this is the component that translates strings displayed in the input field and NgbDateStruct) also does some form of validation, and doesn't return a date struct if the input is invalid. I know that client side validation is generally a good thing, it's just that I felt that this might already be enough - or maybe not, let's see how this works out in practice.

@shamoon
Copy link
Contributor Author

shamoon commented Feb 24, 2021

Sure I’ll play with it in next release, I’m sure it’ll be an improvement over current. I’ll just think of my time on this branch as experimentation 😄

@jonaswinkler
Copy link
Owner

my time

Wasting people's time always feels bad :(

At least I can salvage the css for the dark mode calendar, that's something.

@shamoon
Copy link
Contributor Author

shamoon commented Feb 24, 2021

Oh no worries really, thanks. I should just be more patient about waiting for an actual agreed plan. Regardless still fun to work on!

dark mode calendar css FTW 😄

@jonaswinkler
Copy link
Owner

jonaswinkler commented Feb 25, 2021

New version's on the way, let me know what you think about the date input.

@shamoon
Copy link
Contributor Author

shamoon commented Feb 25, 2021

New version's on the way, let me know what you think about the date input.

This is a major major improvement! As usual, great work. I do think the mask had some niceness about formatting keyboard input but honestly probably not worth it considering how much cleaner this is. Bravo 👏🏼

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

Successfully merging this pull request may close these issues.

2 participants