jtsage / jtsage-datebox

A multi-mode date and time picker for Bootstrap (3&4), jQueryMobile, Foundation, Bulma, FomanticUI, and UIKit (or others)
http://datebox.jtsage.dev/
Other
474 stars 166 forks source link

useTodayButton and usePlaceholder options not functioning correctly in calbox #479

Closed CapDev closed 5 years ago

CapDev commented 5 years ago

Found after upgrading from 4.3.1.

Using jtsage-datebox.bootstrap.js version 5.1.6 for Bootstrap 3.4.1.

Occurs in the Chrome browser version 77 on macOS.

Codepen of issues: https://codepen.io/CapDev/pen/ZEEWdWp

The useTodayButton option produces the opposite result as the api description.

"This button will "select" today, but it will not "set" today. It is for quick navigation."

Expect the "Jump to today" button to shift the calbox month to the current month but this does not occur and instead the date input is set to current date which is not expected.

The usePlaceholder option is only functioning when a simple html input label is used. If the label "for" attribute and the id for the calbox are the same then unexpected results occur.

My datebox use is often within forms written in ruby where the label and datebox id will be the same and changing only the label to html complicates localization.

Also when the usePlaceholder option is allowed to work with a simple html label used, as shown in the codepen above, its value will be passed to the Header text in the calbox. I would expect this text to still be the label value.

Thanks for looking into this.

Thanks.

jtsage commented 5 years ago

Thanks. I'll look at this tonight. I did roll .6 out to release, this will be .7

~j

jtsage commented 5 years ago

Re: placeholder.

So, I think I understand your thoughts here - right now, if usePlaceholder is a string, that gets used as the header text. You are saying that usePlaceholder shouldn't propegate that far, that the text inside <label> should still be used? And the usePlaceholder text should only be used at the placeholder (which is a bit implied by this option name I would admit).

I can certainly see the use case - I'm guessing you are translating the <label> text and not the placeholder itself?

As for the useTodayButton behavior - yeah, that's not right. I can understand why I did it this way, but it's not even refreshing the display like it should. I'll work on making it a true display jump instead of a set the date function.

jtsage commented 5 years ago

Oh, and if the <label> has a correct "for" attribute, the usePlaceholder text gets ignored. Or maybe overridden. Yeah, that's not intended.

CapDev commented 5 years ago

Yes I do not want the usePlaceholder string to overwrite the header text by default.

The placeholder text is sometimes set dynamically which is why using plain html inline is not suitable also.

Correct that the label text is translated in the form.

jtsage commented 5 years ago

Alrighty. Makes sense.

Current order of precedence:

  1. options.overrideDialogLabel
  2. <input> placeholder attr
  3. <input> title attr
  4. <label> text (with proper for attr)
  5. i18n.titleDateDialogLabel / i18n.titleTimeDialogLabel (mode dependant)

So, here is my proposal:

New options, defaulting to true, to preserve what it has done, in order of precedence:

So, your use case, you'd set "headerFollowsPlaceholder: false" (if title is empty, no need to explicitly ignore)

Thoughts?

jtsage commented 5 years ago

The commit above is a fix for the jump to today/tomorrow button. (change pushed to -latest so that part of your codepen is working in that respect)

CapDev commented 5 years ago

I agree with your proposal for the new options that will set the header text.

Tested your change to the useTodayButton function and it is now working as previously in 4.3.1 and how I had thought was intended. Thanks.

jtsage commented 5 years ago

Codepen of an example: https://codepen.io/jtsage/pen/abbZXKv

Built to -latest. Gonna document it and give some test time then run the release (minor dot this time as this is presumably new functionality)

CapDev commented 5 years ago

The options in your codepen example work great. Will further test as I roll through updating my use in app. Thanks for sorting this so quickly.

jtsage commented 5 years ago

No worries. I use such a limited set of these options in my own projects, always nice to hear from someone who is using more of them. Fwiw, the quick turnaround is good for a couple more days, then $realJob is gonna take over for a week or two.