-
Notifications
You must be signed in to change notification settings - Fork 242
[Bug] Fix meaning of control vs control action #1537
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
base: main
Are you sure you want to change the base?
Conversation
…_meaning_of_control_vs_control_action
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2026-01-09 09:40:23 UTC Compare the examples using the commit's wheel file vs the latest released version: vizro-core/examples/scratch_devView with commit's wheel vs View with latest release vizro-core/examples/dev/View with commit's wheel vs View with latest release vizro-core/examples/visual-vocabulary/View with commit's wheel vs View with latest release vizro-core/examples/tutorial/ |
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.
Pull request overview
This PR fixes a bug where vm.Filter stops filtering when its selector's actions are overwritten. The fix separates the filter's internal filtering logic from the selector's action behavior by storing the filter function directly on the Filter model as a private attribute _filter_function.
Key Changes:
- Adds
_filter_functionprivate attribute to Filter model to store the filtering logic independently of selector actions - Refactors
_apply_filter_controlsand_get_parametrized_configto use newget_selector_parent_controlutility - Updates scratch_dev test app to demonstrate the fixed behavior
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| vizro-core/src/vizro/models/_controls/filter.py | Adds _filter_function private attribute and refactors pre_build to always set it, regardless of whether selector actions are overwritten |
| vizro-core/src/vizro/models/_controls/_controls_utils.py | Adds get_selector_parent_control utility function to retrieve parent control (Filter or Parameter) from a selector |
| vizro-core/src/vizro/actions/_actions_utils.py | Refactors _apply_filter_controls and _get_parametrized_config to use get_selector_parent_control and access filter function from Filter model |
| vizro-core/examples/scratch_dev/app.py | Updates test app to demonstrate the bug fix with filter behavior |
| vizro-core/changelog.d/20260108_150147_petar_pejovic_fix_meaning_of_control_vs_control_action.md | Adds empty changelog template (needs to be filled out) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...o-core/changelog.d/20260108_150147_petar_pejovic_fix_meaning_of_control_vs_control_action.md
Show resolved
Hide resolved
huong-li-nguyen
left a comment
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.
I might be missing a bit of context, but based on the PR description I thought the behaviour is now: Custom action will be appended to the default action of filtering for the Filter for example.
Based on that understanding, i would have thought that the Filter would apply automatically with the custom action added on top. However, in the scratch app, the filtering is only applied after the parameter action is also applied, so it's not the behaviour I would have expected 🤔
Is this intended? What I would have expected is that the filter is ALWAYS filtering and then adding any custom action on top. So in the scratch dev app, I would have expected it to filter regardless whether i click on the parameter action or not.
| --> | ||
| ### Fixed | ||
|
|
||
| - Fix that `Filter` and `Parameter` logic is always applied on target figure refresh, even if custom selector actions are defined. ([#1537](https://github.com/mckinsey/vizro/pull/1537)) |
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.
| - Fix that `Filter` and `Parameter` logic is always applied on target figure refresh, even if custom selector actions are defined. ([#1537](https://github.com/mckinsey/vizro/pull/1537)) | |
| - Custom actions for `Filters` and `Parameters` now run in addition to the default behavior, rather than replacing it. Filtering and parameter updates will always be applied as expected. ([#1537](https://github.com/mckinsey/vizro/pull/1537)) |
| ), | ||
| ), | ||
| vm.Text(id="p11_text", text="Placeholder"), | ||
| ], |
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.
Do we need to add some tests for this?
|
Just a note to say that I've seen this PR and #1538. I'm going to think through both of them properly next week as well as finally go back to my notes to find the answers to these questions you asked:
I spent a looooong time last year thinking about about implementing something very similar to #1538 and ultimately decided not to. Possibly that was the wrong decision, but I'd like to go back and find my earlier thoughts on it to understand what I was thinking before anyway! |
closes https://github.com/McK-Internal/vizro-internal/issues/2421
Description
This PR introduces a clear distinction between
vm.Filterandfilter_action(same for parameter). What does it mean?Here's the configuration of the
vm.Filterfromscratch_dev/app.py:On the
mainbranch, overwritingvm.Filter.selector.actionscauses the filter to stop filtering, which is incorrect.vm.Filteris meant to represent filtering logic, and it should always be applied whenever its targets are refreshed (via OPL or any other action), regardless of whether the selector’s actions are overwritten.So, the
vm.Filter.selector.actionsdefines of what happens when the filter’s trigger property changes, as with any other model. Forvm.Filter, the default behaviour is to refresh the targeted figures (it's just a default behaviour). If actions are overwritten, the custom behaviour should run instead. However, the filtering must still be applied when the target figures are refreshed.Also, this PR prepares the ground for replacing the private
_filter_actionwith the new publicupdate_figuresAPI by clarifying responsibilities and reducing ambiguity in the filtering model.Screen recording from the
main:❌
vm.Filteris NOT taken into account whenp11_graphis updated fromon_page_loadorparameter_action.Screen.Recording.2026-01-08.at.14.13.43.mov
Screen recording from the
bug/fix_meaning_of_control_vs_control_action:✅
vm.Filteris taken into account whenp11_graphis updated fromon_page_loadorparameter_action.Screen.Recording.2026-01-08.at.14.15.03.mov
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":