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

Various improvements to the DateFilter widget #276

Merged
merged 5 commits into from
Aug 11, 2022
Merged

Conversation

maximlt
Copy link
Member

@maximlt maximlt commented May 12, 2022

Includes various fixes to the DateFilter:

  • On picker mode cast start/end/value to datetime.date
  • Override the value Parameter by a CalendarDate or DateRange Parameters (let me know if there's a better way, that looks hacky!)
  • Bidirectionally link the widget to the value Parameter, this allows URL query parameter changes to be synced back to the widget.

This is work in progress and requires other changes both in Param and Panel. I've also noticed accumulating calls in get_data on picker mode so there's something wrong there.

@maximlt maximlt changed the title [WIP] Various improvements to the DateFilter widget Various improvements to the DateFilter widget May 18, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #276 (f92a7cb) into master (99a62e3) will decrease coverage by 0.19%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #276      +/-   ##
==========================================
- Coverage   65.42%   65.22%   -0.20%     
==========================================
  Files          57       57              
  Lines        5611     5628      +17     
==========================================
  Hits         3671     3671              
- Misses       1940     1957      +17     
Impacted Files Coverage Δ
lumen/filters/base.py 63.07% <0.00%> (-2.70%) ⬇️
lumen/ui/variables.py 36.36% <0.00%> (-2.20%) ⬇️
lumen/ui/launcher.py 47.25% <0.00%> (-2.18%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99a62e3...f92a7cb. Read the comment docs.

@maximlt
Copy link
Member Author

maximlt commented May 18, 2022

This is ready for review @philippjfr

The accumulating calls to get_data I saw when picking a date were due to an issue in Param. The last commit just allows to link visible and disabled to the date widget, they were not linked which looks like an oversight.

A few other PRs were required to get this properly working:

@maximlt
Copy link
Member Author

maximlt commented Aug 2, 2022

@philippjfr I've reworked this PR quite a bit, c89cb89 is where you can see all the changes I've made.

Based on our discussion and the write-up in #293, I've implemented multi for the date filters, and I've added datetime filters. The implementation has changed quite a bit as instead of changing the value Parameter with self.param.add_parameter, I implemented __new__ to dispatch to classes that have override value with the right Parameter type.

There are a couple of decisions I've made which we'll need your approval:

  • For multi=True and mode='picker', as there's no DateRangePicker available in Panel yet I've decided to fallback to a DatetimeRangePicker and emit a warning. This is the option 2 I mentioned in Meta-issue on date filters/widgets across Lumen, Panel and Param #293
  • For multi=False and mode='slider', as there's no DatetimePicker I've decided to emit a warning and fall back to a DatetimePicker

It'd probably be worth:

  • Adding some tests!

@philippjfr
Copy link
Member

Adding tests separately. Thanks @maximlt!

@philippjfr philippjfr merged commit 88127c9 into master Aug 11, 2022
@philippjfr philippjfr deleted the sync_datefilter branch August 11, 2022 08:19
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 11, 2023
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.

3 participants