ghiscoding / Aurelia-Bootstrap-Plugins

Aurelia-Bootstrap-Plugins are Custom Elements to bridge with a set of 3rd party Bootstrap addons
https://ghiscoding.github.io/Aurelia-Bootstrap-Plugins
MIT License
45 stars 23 forks source link

Time-only picker loses date portion of original value #84

Open jordan-ware opened 5 years ago

jordan-ware commented 5 years ago

I'd label this is more of an enhancement, but:

If you bind two controls to the same date, e.g. one control with a date format of DD/MM/YYYY and one with a time format of hh:mm a, the second control will end up losing the date portion of the original value as it reads it's own strings from the input value and then syncs with the model property.

I think the optimal solution here is to have an extra internal property that actually binds with the bootstrap datetimepicker input, e.g. internalValue which will then publish any input changes to the bindable model and value properties that are exposed. Using this method we can ensure that the date portion is not lost during the sync process.

The same method of identifying if it's only a time value can be taken from the underlying datepicker library (see their hasDate, hasTime and isEnabled functions). This can be used to check for this scenario and ensure that the date portion is not lost.

An example of the issue below in the log statements, you can see as valueChanged triggers an update on model, it loses the date portion and resets to the current date (12th of February in this case).

I'm starting to get an understanding of this wrapper, I might be able to get a PR around this.

image

ghiscoding commented 5 years ago

Just curious, why would you bind 2 controls to the same date?

On another note, all the plugins I made have both the model and the value but seriously it's a pain to maintain them and it's easy to get into an infinite loop. I think there's still an open issue for an infinite loop actually.

BTW, nice image.. Tintin 😺

jordan-ware commented 5 years ago

I had a project previously where users did not like the combined input on one field, so we separated it out. I think I was using this library and it somewhat worked - but if the user deleted the time input then it would reset.

I understand your pain, I added in support for selecting objects instead of values for a selectize wrapper: https://github.com/radical-systems-australia/au-selectize/blob/master/au-selectize.ts Note, still a work in progress :sweat_smile:

and yeah, I get comments in real life that I look like Tintin :laughing:

ghiscoding commented 5 years ago

Ah Selectize was another lib I was looking at, they also have an extension to make something like the bootstrap tagsinput (this one I made but I later found out it doesn't handle array of objects). The Selectize has this "remove_button" plugin

"remove_button" This plugin adds classic a classic remove button to each item for behavior that mimics Select2 and Chosen.

... nice to meet you Tintin haha... which city are u? I went to Sydney 2 times, my best friend moved there.

jordan-ware commented 5 years ago

Yeah, I've got a binding multiselect which, when true, will give you that mode. Here's a preview, I'm currently fixing selectedValues & selectedObjects listening for changes from the parent component. I'm making sure that if you reassign an array, or mutate it, all changes are detected and represented in the control. Should be ready in the next 2 hours I think. It's proving to be a big pain trying to sync two arrays that are listening to eachother :laughing: image I'm from Darwin, which is in the Northern Territory. It's a relaxed life style here :slightly_smiling_face:

ghiscoding commented 5 years ago

Yeah if I was to rewrite the plugins I made, I would drop the 2 properties and only keep 1, mainly the object itself. If a user wish to get just the id, then we could provide an extra option/method to simply get the property from that given object. I had way too many infinite loop issues opened up in this project and it's hard to fix and troubleshoot, though I know they all come from the fact that it is hard to keep the 2 in sync.

Your plugin looks quite promising, I'm probably going to use it as well. If you wish I could also add a link to your lib into my lib demo, to give you more visibility. Lately most of my time is spent on my other lib Aurelia-Slickgrid (and it's cousin in Angular). Those are written in TypeScript and having the time, I would rewrite all the Bootstrap Plugins as well in TS... but yeah free time is also rare.

Darwin, wow that is quite far up north, isn't that quite warm in that area? I see you're not too far from Indonesia. Ahh I wish I could live in Australia too, seriously I'm Canadian and winters are way too long ⛄️ I'd like to visit more of Australia next time I go, maybe NZ too, seems nice as well.

jordan-ware commented 5 years ago

I'm starting to feel confident in handling the syncing issue, would be happy to enhance the sync in the datetimepicker when I have the time, maybe as a separate PR.

Also I think an internalValue would allow the support for providing both a displayFormat and a valueFormat, for example I might want to display the value as DD/MM/YYYY hh:mm a but sync back as the ISO standard for when I want to use it against a web service.

I just pushed an update to au-selectize and it's properly syncing now. It was made exceptionally harder by 1. having to sync arrays and 2. listening for mutations as well as assignments, meaning I had to duplicate the logic into a collectionObserver as well as the standard propertyObserver pattern. Should be very stable for use now, I tested it pretty thoroughly :slightly_smiling_face: I'd be happy for you to include a link in your demo, ideally I'll have it set up as an NPM package soon. My colleagues and I really love Aurelia, and want to help the community grow by having these ready to use wrappers.

I have a friend who moved to Canada and it looks amazing. I have no solid plans yet, but definitely hope to go there soon. I went to NZ for 3 weeks last year, just hired a car and went around doing hikes and finished the trip with snow boarding. The country is absolutely beautiful and I would highly recommend it! It's pretty much opposite of Darwin, here it is hot, red and flat. There it is green, cool and hilly/mountainous.

ghiscoding commented 5 years ago

I merged your code and just released a new version of the DateTime Picker, version 1.3.7, there was also another fix for #83 that I pushed as well (changing the input value.bind to a value.two-way="value & updateTrigger:'blur'" to help with typing in the input).

While I was testing, I found an issue and I'm not sure if I should mention it or not (because it's kinda stupid) but I created myself an infinite loop (or kind of, since I have a throw error in place preventing it). Anyhow, the bug I created in my own code was to point the value.bind and the model.bind to the exact same variable, causing well... the infinite loop. I don't think there's any way to prevent this though.

If you have any improvement that you want to do to the plugin, please feel free to create PRs. I'm quite open on this and I have to admit that I'm a bit behind with Aurelia since we use Angular at work (not my decision and it sux). These Bootstrap Plugins were the first ones I've made, that is why they are in plain ES6 instead of TypeScript, nowadays I'm all for TS everywhere.

Also I think an internalValue would allow the support for providing both a displayFormat and a valueFormat, for example I might want to display the value as DD/MM/YYYY hh:mm a but sync back as the ISO standard for when I want to use it against a web service.

That is actually why I wanted to provide the model and the value, so that we can still do another format while saving, but I guess what you're talking about is like a 3rd property 😮

I'd be happy for you to include a link in your demo, ideally I'll have it set up as an NPM package soon. My colleagues and I really love Aurelia, and want to help the community grow by having these ready to use wrappers.

I just tried to build the plugin demo and it's actually broken, not sure when I'll have time to look at this. I'll probably need to redo it completely. I remember that Aurelia was breaking often over a year ago, I think they're more stable now and it's probably best to redo a new demo project with all new packages of Aurelia lib to get it up and running. In the meantime, if you have an NPM package, I can add a link in my lib README until I get the demo building again.

jordan-ware commented 5 years ago

Hopefully this clears it up a bit. So...

binding model

inputValue would bind to the text box. On update, it will sync with the model, which stores the date object. It can have special logic to not override the date portion of model, if the display format is only time.

You can probably add a new valueFormat or something, for users who will be binding to your control using a string, e.g. in my case it's ISO.

I understand the changes are not trivial, just an idea for now and something I can probably try a PR for in future. Would aim to not change any binding names to ensure it's backwards compatible.

Hope this makes sense.

Edit: just to clarify, I changed a few details from what I was explaining before because I hadn't quite thought it through completely