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

Flatpickr #9509

Merged
merged 14 commits into from Jan 17, 2020
Merged

Flatpickr #9509

merged 14 commits into from Jan 17, 2020

Conversation

bryevdv
Copy link
Member

@bryevdv bryevdv commented Dec 6, 2019

Replace the current date picker with Flatpickr and also make the Date property more strict.

  • issues: fixes #9494
  • issues: fixes #6728
  • issues: fixes #8917
  • tests pass
  • docs udpated

@bryevdv bryevdv changed the title Faltpickr Flatpickr Dec 6, 2019
@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Dec 6, 2019

As an aside I have realized half the issue is that our handing of date values (by forcing them to be datetimes) is half the problem(s). It did not help that is what pikaday expected. But Flatpick can handle actual date values with no no time, e.g. "2019-12-06", so I cam going to fix our Date property and serialization to no longer use timestamps.

@bryevdv bryevdv force-pushed the bryanv/flatpickr branch 2 times, most recently from 7fb3001 to 22a13dc Compare Dec 25, 2019
@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Dec 25, 2019

@mattpap I'm not actually seeing what JS tests fail:

       ✓ should should leave message null
        ✓ should throw an error on binary data
[18:03:12] failed: Error: tests failed
[18:03:12]     at ChildProcess.proc.on (/Users/bryan/work/bokeh/bokehjs/make/tasks/test.ts:99:16)
[18:03:12]     at ChildProcess.emit (events.js:182:13)
[18:03:12]     at Process.ChildProcess._handle.onexit (internal/child_process.js:240:12)

Is there something I need to do or does this need a separate issue?

@mattpap
Copy link
Contributor

@mattpap mattpap commented Dec 28, 2019

I'm not actually seeing what JS tests fail

Test failures are currently showed only inline. We already discussed this, though there may not be an issue for this, and I'm going to improve reporting as time permits. If you don't see any errors inline and tests fail, then report a new issue.

@bryevdv bryevdv force-pushed the bryanv/flatpickr branch 7 times, most recently from 3aefe8b to c0c04e1 Compare Dec 30, 2019
@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Dec 30, 2019

@mattpap @philippjfr this is still WIP but I would like some feedback. First, things to do:

  • expose further features of flatpickr
  • add more selenium tests

Both of those will be fairly easy/straightforward. I'd like some input on the properties changes. The current code around serializing dates and datetimes is a mess, so the current changes are also a mess, and that is what I would like input on.

I think ideally what should happen is for scalar datetime.date values to render as ISO datestrings. These would be for individual Date properties, e.g. the value of the picker. But for everything else (i.e. date objects in arrays/series/columns) to transform as timestamps. Does this seem reasonable?

Currently this is not quite true -- series/arrays transform as timestamps but plain lists of dates become iso strings:

In [6]: serialize_json(df.Date[:5])
Out[6]: '[1356998400000.0,1357084800000.0,1357171200000.0,1357257600000.0,1357344000000.0]'

In [7]: serialize_json(list(df.Date[:5]))
Out[7]: '["2013-01-01","2013-01-02","2013-01-03","2013-01-04","2013-01-05"]'

I think the list case can be made consistent, though.

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Jan 13, 2020

I think the list case can be made consistent, though.

Well, I was wrong about this, but I think we just document it and live with it.

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Jan 13, 2020

@philippjfr can you test this with HV/Panel when you get a chance?

@bryevdv
Copy link
Member Author

@bryevdv bryevdv commented Jan 13, 2020

@mattpap @philippjfr OK I think everything except plumbing to hook up whatever set of flatpickr features we want, is done. I hope to have this PR done done by tomorrow night.

I think the Date/Datetime property situation is generally better. To summarize:

  • Date accepts dt.date values and ISO date strings ("2020-01-11") and, importantly, serializes as ISO datestring. This solves the nonsense with datepicker and time zones.

  • Datetime accepts:

    • python, pandas, numpy datetime objects (including dt.time)
    • dt.date and date strings
    • ms since epoch timestamps

    and still serlializes as ms since epoch

One weird but anavoidable corner case: lists of dt.date get serialized as ISO strings, arrays and series dt.date get serialized as datetime/timestamp

Weird stuff I noticed but did not touch: timedeltas and pandas Period values are serialized as datetimes. I think this is not the right thing to do, in general.

@bokeh bokeh deleted a comment from lgtm-com bot Jan 13, 2020
@bokeh bokeh deleted a comment from lgtm-com bot Jan 13, 2020
@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jan 14, 2020

This pull request introduces 1 alert when merging c3471d9 into 16a7acd - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

else if (isArray(item) && item.length == 2)
result.push({from: item[0], to: item[1]})
}
return result
Copy link
Contributor

@mattpap mattpap Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type DatesList = (string | string[])[]

Based on the above code the type should be (string | [string, string])[]. The second condition is unnecessary. The return type should be (string | DateLimit<string>)[].

Copy link
Contributor

@mattpap mattpap Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be very meaningful to type DateType = string, primarily for readability, even if that doesn't change anything from static typing perspective (TypeScript doesn't have nominal types unfortunately).

Copy link
Contributor

@mattpap mattpap Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type DateType = string

Actually, I will introduce that later, because there's an opportunity here to introduce pseudo nominal types.

private _picker: Pikaday
private _picker: flatpickr.Instance

private _set(key: string, value: any): void {
Copy link
Contributor

@mattpap mattpap Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

key: keyof Options, then you won't need the cast below.

// toISOString() returns the wrong day (IE on day earlier) #7048
// XXX: this should be handled by the serializer
this.model.value = date.toDateString()
_on_change(_selected_dates: any, date_string: string, _instance: flatpickr.Instance): void {
Copy link
Contributor

@mattpap mattpap Jan 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@bryevdv bryevdv Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattpap the only way I can get those types to import is to do

import {Options as FlatpickrOptions, DateLimit} from "flatpickr/dist/types/options"

which does seem safe/good (importing from dist). Tried installing @types/flatpickr but trying to import that also yields an error. The only thing exported from standard flatpickr seems to be the flatpickr instance.

Copy link
Contributor

@mattpap mattpap Jan 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a common mistake that library author make, i.e. not exporting publicly enough type information. Let's leave it for now.

@lgtm-com
Copy link

@lgtm-com lgtm-com bot commented Jan 17, 2020

This pull request introduces 1 alert when merging ec557b6 into 87ca243 - view on LGTM.com

new alerts:

  • 1 for Module is imported with 'import' and 'import from'

this.connect(this.model.change, () => this.render())

const {value, min_date, max_date, disabled_dates, enabled_dates, position, inline} = this.model.properties
this.connect(value.change, () => this._set("defaultDate", value.value()))
Copy link
Contributor

@p-himik p-himik Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bryevdv This sets only the initial date. The current date will not be changed. See the report at https://discourse.bokeh.org/t/datepicker-ui-not-updating-in-bokeh-2-0/5021

Copy link
Member Author

@bryevdv bryevdv Mar 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p-himik OK we should get a fix in for 2.0.1 (with a test) can you make an issue and I will take a look tomorrow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment