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

showTodayButton showing; timePicker is not showing #1

Closed don-bluelinegrid closed 7 years ago

don-bluelinegrid commented 7 years ago

Using the default/simple setup, I am seeing the "Today" button below the calendar, and am not seeing the "Time" button - so there is no access to the time picker. See the screen shot attached.

Please advise on why this is not working, and how to get it to work properly as expected.

Thanks, Don screen shot 2017-03-16 at 3 38 54 pm

don-bluelinegrid commented 7 years ago

This is the markup:

<abp-datetime-picker element.bind="picker" value.bind="user.birthdate"  format="YYYY-MM-DD"></abp-datetime-picker>                        <!--<input datepicker.bind="{ minDate: -20, maxDate: 20 }" value.bind="dateValue">-->

I have also tried adding showTodayButton="false" but that makes no difference.

ghiscoding commented 7 years ago

I think it's because I've changed the default format to be this @bindable format = 'YYYY-MM-DD'; and in the doc it says

format: Default: false See momentjs' docs for valid formats. Format also dictates what components are shown, e.g. MM/dd/YYYY will not display the time picker.

The most important part is the last sentence which says MM/dd/YYYY will not display the time picker. So in your case, if you try to put your format back to the default (that is false in the docs) then you most probably will see the time show up... or simply use a moment format that has time (hour/minutes) like YYYY-MM-DD hh:mm. I will remove any defaults in my next version to remove any inconsistencies.

IMPORTANT NOTE I'm about to change completely how to pass the Datetime-Picker options, having all options as bindable is not ideal since the library might add new options and my plugin would be out of sync. I will very soon remove all bindables and instead use picker-options.bind="myOptions" or picker-options="{ format: YYYY-MM-DD, mindDate: false, ...}". This will happen in a week or two, I will release it under a major version change, most likely 1.0.0, so stay tuned.

I'm currently working on another plugin Bootstrap-Select and that one uses the new technique of picker-options. This plugin took a lot of my time, however I'm about to be done with it and when that happens I will change the other 2 plugins to be using same option format.

don-bluelinegrid commented 7 years ago

Thanks for that.

I changed the date format, and now I do see the "Time" button - but I cannot make the "Today" button go away. I've added the dateFormat, and I've tried adding or removing showTodayButton="false" and nothing seems to make a difference. So, how do I eliminate the "Today" button?

Thanks.

don-bluelinegrid commented 7 years ago

Also, it seems that sideBySide="true" has no effect as well.

ghiscoding commented 7 years ago

As I mentioned earlier, all options are bindable values, the option showTodayButton in the View has to be written with - so it should show-today-button="false" in the View

don-bluelinegrid commented 7 years ago

Ah, I see.

Yes, I agree with your other approach then - that's much more consistent with the use of config/options objects in the frameworks, and much more backward/forward compatible.

Thanks, Don

don-bluelinegrid commented 7 years ago

OK, now i am seeing an error at runtime in the browser console.

I'm using side-by-side="true" and am receiving this error:

Unhandled rejection TypeError: sideBySide() expects a boolean parameter at Object.picker.sideBySide (http://localhost:9000/app.bundle.js:76239:23) at String.<anonymous> (http://localhost:9000/app.bundle.js:75817:32) at Function.each (http://localhost:9000/app.bundle.js:28257:19) at Object.picker.options (http://localhost:9000/app.bundle.js:75815:15) at dateTimePicker (http://localhost:9000/app.bundle.js:76679:16)

ghiscoding commented 7 years ago

Yes I will change it soon, if I can finish the other Bootstrap-Select plugin.

However don't forget that in Aurelia, what is written in camelCase in the ViewModel has to be written in kebab-case in the View. Here's a good article talking about this Don’t Get Skewered By “kebab-case” In Aurelia

don-bluelinegrid commented 7 years ago

Also, you may want to take a look at ag-grid, which is a very mature 3rd-party plugin, with Aurelia support, for how it uses custom elements and the attributes in the element:

https://www.ag-grid.com/javascript-grid-getting-started/?framework=aurelia#gsc.tab=0

They take the approach of having the presence of any attribute signify a boolean true for that attribute; using XXX="false" is only required when exlplicitly overriding a default option value.

ghiscoding commented 7 years ago

Hmm it seems that using bindable is causing more issues that I thought.

Try adding the .bind to your option, that will make sure it converts to a boolean value. Like this side-by-side.bind="true"

don-bluelinegrid commented 7 years ago

Another question/issue:

I've now got things working, and have the date/time result bound to a model object. I'm noticing that the value bound to the model is the same literal value as what is displayed in the intput field. In other words, the formatting is being applied to both the display and the bound data. From reading the docs, that is what's happening, and this does not seem correct. The view should be responsible for formatting a known data type into a string to be presented to the user in the view. In this case, I'd expect the known data type to be a JS Date object.

Having a formatted string like 2017-03-15 17:30 isn't really helpful when we need to send a standard date representation to a server-side REST service. Is there a way to get a normal date representation in the bound value, like a UNIX time value? I know that we can format the moment object with 'X' to get the UNIX time, but that is not what I'd want to display in the user-facing input field.

Thanks, Don

ghiscoding commented 7 years ago

You mean that you want the Date object representation? I don't currently have that, but I could possibly add a model attribute to keep that. Does that sound good having value.bind="" as the text formatted version and the model.bind="" as the Date object? Or if you have other suggestions

ghiscoding commented 7 years ago

I made a quick change to add a model.bind which now holds the Date Object. I've publish this quick version 0.3.3 on NPM. I didn't updated the README since I'm going to refactored all the bindables eventually.

Here's an example

<abp-datetime-picker value.bind="post.dateEntered" model.bind="myDateObject" format="YYYY-MM-DD hh:mm"></abp-datetime-picker>

The myDateObject is the Date Object while the post.dateEntered is the formatted date.

If that fixes your issue, then please close it. If you like the plugin, you can also click on Like/Star. Cheers

don-bluelinegrid commented 7 years ago

Not exactly -

Here is what I'm suggesting:

I think that generally the intention is that model.bind in a component/custom element should bind the result/output of the component to a ViewModel property. So, in your case, the date data that is produced by the component should bound to the ViewModel's property, via model.bind. And, I'm suggesting that this data should be in a standard data-type; a standard data-type for a date would be preferably a JS Date object, and secondarily a UNIX-style long number.

So, the component's data should be bound through ``model.bind```, as you suggested. For the display element, this should also be using the some data value - everything should be bound to a single instance of the data. However, for the view's display element, I believe that the intention would be to use the framework's binding formatter capabilities to produce the desired view format.

So, for example, we could have something like this:

                    <abp-datetime-picker element.bind="picker" model.bind="controller.dialogController.alertModel.alertSchedule.sendTime" 
                         format="YYYY-MM-DD HH:mm"></abp-datetime-picker>

The format property above should be used in the string interpolation to use the built-in date-formatting in Aurelia.

Don

don-bluelinegrid commented 7 years ago

In response to your comment above:

There really should be no need to use ```value.bind="XXX"

ghiscoding commented 7 years ago

Well I want to leave the value.bind as the formatted date, since I most often want that formatted date and value.bind is most often what I've seen in custom element use. As for the Date object, I used moment to return the object. If the model.bind is not the correct attribute name to use then propose another one.

I need to go out now, so I won't do more changes for today.

don-bluelinegrid commented 7 years ago

OK, I've tried out your version 0.3.3. Thanks for making the update so quickly.

This is basically working to return an object in the model.bind. However, it is returning a moment object, not a native JavaScript Date. It really should be returning a native JS Date object, since moment is a plugin, not native JS. You can just call moment().toDate() to return the real JS Date object.

Thanks, Don

ghiscoding commented 7 years ago

Ok I changed it to moment().toDate() and published an NPM version 0.3.4. Give it a try.

Do you have other suggestion for the naming, instead of model.bind? I would like to use the same naming across my plugins. I'm just hesitating on the name, I'm not sure what would be the best naming to return the object. I would like to most often return the id and the object itself, as for example with Bootstrap-Select it's convenient to know the selected id and the selected object so that I can pull/display a certain property of this object.

thejaff2 commented 7 years ago

Just my 5 cents (but first, thanks for the plugin!):

Aurelia works fine with ValueConverters, so the way I have been working with Dates is like this: <abp-datetime-picker value.bind="startdate | dateFormat" format="YYYY-MM-DD"></abp-datetime-picker>

where dateFormat is:

import * as moment from 'moment';
export class DateFormatValueConverter {
   toView(value: Date, format: string = "YYYY-MM-DD"): string { 
    return moment(value).format(format)
  }

  fromView(str: string, format: string = "YYYY-MM-DD"): Date {
    return moment(str, format, true).isValid()
      ? moment(str, format).toDate()
      : null;
  }
}

Another thing, that is behaving differently from standards input fields: if you declare and assign a date immediately in the viewmodel, the initial binding wont "take". But if you do it in for example the attached() event it will..

ghiscoding commented 7 years ago

@thejaff2 This is an interesting way. Please correct me if I'm wrong in the text below. (note: I'm still new to Aurelia and have done rather small project until now).

If I understand correctly, not adding the value converter, would mean that the value.bind is a Date object? This could be confusing, especially for newcomers and the fact that there's also a format for the datepicker itself. Using it this way would also mean that you need to pass a Date object in the ViewModel as well if you want to initialize the picker, isn't it? If so, then that's also adding to the confusion. I would rather have 2 separate bindable values to differentiate both as every project/user are different usage and sometime we need to deal with a Date object while other time just the formatted string would suffice.

For the last issue you mentioned, I will use bind() in the next major version to make it in sync.

The other plugin that I'm working on, is just taking too much of my time to properly spend the time to review this one here. However I will try to review this one in the coming weekend. This is done on my free time, so any help is also appreciated and I would be happy to accept any PR.

don-bluelinegrid commented 7 years ago

@ghiscoding @thejaff2

thejaff- Please take a look above - Ghislain has already made a change to return a model object as a JavaScript Date.

Ghislain and thejaff -

In JavaScript, dates are explicitly and intrinsically represented using a Date object - this is why the Date object exists. We should not subvert or re-invent this. Once we have a Date object, we can very easily format this for display in any way we like, using formats. This is no different than a Number object, where we can format a native Number as a percent, a decimal, currency, etc., and no different from how these object types are handled in other languages. The String value is for display only; the native object is used as the data model. We should not be using a formatted string as a data model, for obvious reasons.

Now that Ghislain has made the change to provide the Date object in the model, I would consider this closed, with the exception of standardizing the naming/bindings. Any developer can then freely format the display value as they wish, without the need to parse first.

Thanks, Don

ghiscoding commented 7 years ago

@don-bluelinegrid @thejaff2 , thanks for the comments. I would just like to know if the 2 available bindable are ok with the current naming or if you guys suggest other names for the next major release.

Currently the naming from last version available (sorry I didn't update the readme yet) is

value.bind="myFormattedDate" // returns the string formatter, same as the datepicker
model.bind="myDateObject" // returns a Date object

Please confirm the naming now so that I can make the changes in the next release (which will be a major). So we should confirm the naming now, please. Thanks.

I'm wondering if it would be better to have the value.bind return the Date object, while using something else for the string, like possibly formatted-date.bind="myFormattedDate" is that better or worst?

Note that the next release will be a major release 1.0.0 because there will be breakable changes (removing all picker bindable options and replacing with picker-options.bind="{ yourOption: ... }" to be standardized).

thejaff2 commented 7 years ago

Most (all?) html-inputs use strings as their value and it is up to the developer to parse this value. Being able to both use value.bind (as string) and model.bind (as object) covers all bases, thumbs up from me.

ghiscoding commented 7 years ago

Closing since I just released a major version 1.0.0 on NPM. Make sure to rewrite your options call by the new options.bind for example options.bind="{ format: 'YYYY-MM-DD' }". Give it a try and see if it's all good... Also both value.bind and model.bind are two-way binding so any of the 2 will/can affect the picker.

There's a lot of changes, like new Global Options, so take a look at the README.

I will update the Client samples (CLI/Webpack) tomorrow

Thanks for the constructive feedback.

ghiscoding commented 7 years ago

There was a bug in version 1.0.0-1.0.2, I pushed a fix in release 1.0.3 so please update NPM to latest version 1.0.3 of aurelia-bootstrap-datetimepicker.

Also release a new plugin Aurelia-Bootstrap-Select on NPM / source Bootstrap-Select

Upvote if you like it, thanks

don-bluelinegrid commented 7 years ago

@ghiscoding

Hi,

I've just now updated to 1.0.7 after the various changes, and I'm seeing a problem.

The expectation with the two-way bound model.bind would be that the control should read the current value and display it appropriately, and should also communicate changes in the control back to the model value.

I'm seeing a problem with the first case, reading the value. I have a model object with a dueDate property, of type Date. When I include the datepicker element and bind it to this property, it is throwing an error:

Uncaught Error: Datetimepicker, model.bind must be of type Date

When I debug through the ABP code, I see that the error is being thrown here:

  AbpDatetimePickerCustomElement.prototype.modelChanged = function modelChanged(newValue, oldValue) {
    if (typeof newValue.getMonth !== 'function') {
      throw new Error('Datetimepicker, model.bind must be of type Date');
    }
    if (newValue !== oldValue && newValue) {
      if ((0, _moment2.default)(newValue, this._format, true).isValid()) {
        this.value = (0, _moment2.default)(newValue, this._format, true).format(this._format);
      }
    }
  };

What seems to be happening is that this function modelChanged is being called three times - the first two times it's called with Date objects for newValue and oldValue, but the third time it's called with newValue as a string "Wed May 31 2017 15:15:22 GMT-0400 (EDT)", which appears to be the Date.toString() of the original Date object. So, this fails the test above. See the attached screen shot. screen shot 2017-06-05 at 11 13 46 am

My code is not doing anything special here. I'm simply including the abp-datetimepicker element and binding it to my model's Date object property.

            <abp-datetime-picker
                element.bind="picker" model.bind="item.dueDate" 
                show-today-button.bind="false" side-by-side.bind="true" format="YYYY-MM-DD"></abp-datetime-picker>

Is there someplace in the ABP code that would be calling modelChanged() after either formatting the model.bind property or calling toString() on it. This is making the component unusable for me currently.

Thanks, Don

robscottnh commented 7 years ago

Did this ever get resolved? I'm having the same issue.

Thanks, Rob

ghiscoding commented 7 years ago

This was resolved a long time ago, if you have any problem then open a new issue with the code you are using.