-
Notifications
You must be signed in to change notification settings - Fork 142
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
[Feat] Enable dynamic filter #879
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…h minimal loading - tests pass
…ter.pre_build - tests pass
for more information, see https://pre-commit.ci
# Conflicts: # vizro-core/src/vizro/models/_controls/filter.py
…_manager to the Filter.pre_build data_manager _multi_load
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! A few minor tweaks but happy to approve ahead of any final changes 🥇 🏆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 ⭐ ❤️ Really cool stuff @petar-qb @antonymilne
I have now gone through the code in detail, tried to understand alll code flow with Debugger, and I think this is great.
Obviously it was hard to grasp all the last details without having been in the meetings, but from what I can understand this is really sound!!
I have not yet looked at the tests, can do later today, please let me know if that is required. I am definitely fine with this already. Well done again!
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
for more information, see https://pre-commit.ci
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Maximilian Schulz <83698606+maxschulz-COL@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Final seal of approval and huge congratulations and applause from me 🎉
@maxschulz-COL
I don't think it's mandatory for you to take a look at the tests. You can quickly skim through them if you want, but it's really optional. |
Description
This PR introduces
dynamic
filters for the following selectors (so, all Vizro selectors exceptvm.DatePicker
):vm.Dropdown
,vm.RadioItems
,vm.Checklist
,vm.Slider
andvm.RangeSlider
You can test the feature by altering values in the
scratch_dev/data.yaml
(in the way that's described in the comment) and running thescratch_dev/app.py
.TODOs:
This PR TODOs:
This or next PR TODOs (we should decide it):
vm.DatePicker
. (Maybe it's better to wait for the dash persistence bugfix as persistence doesn't work for the vm.DatePicker even on themain
branch)Next PRs:
selector.value
handling for new users. There are a few cases marked with 🟠 in this Issue -> https://github.com/McK-Internal/vizro-internal/issues/1356. There are two inconsistencies listed at the bottom of the Issue description. This TODO points to thesecond
inconsistency.multi=False vm.Dropdown
dynamic selector bug when the value is cleared. This will be solved when Vizro universal placeholder component become introduced.data_frame Parameter
default values from themodel_manager
into theDM._multi_load()
that's called from thevm.Filter.pre_build()
. -> PoC can be found in the comment.filter.build
UI for the returned object. -> [Feat] Enable dynamic filter #879 (comment), [Feat] Enable dynamic filter #879 (comment)References:
Open questions:
update_figures(targets=[”filter_1_id”])
data_frame
property for the vm.Filter component, and if thedata_frame Parameters
change the targeting form todata_manager_key.function_argument
, does is mean that our dynamic Filters could be handled in a same way as any other dynamic figure components?Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":