-
Notifications
You must be signed in to change notification settings - Fork 147
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
[Tidy] Prepare for dynamic filters, part 2 of 2 #857
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
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2024-11-11 12:27:23 UTC Link: vizro-core/examples/dev/ Link: vizro-core/examples/scratch_dev |
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.
Awesome refactoring and performance optimising. 🥇🥇 (I left just a few minor comments.)
vizro-core/changelog.d/20241105_170003_antony.milne_new_interaction.md
Outdated
Show resolved
Hide resolved
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.
Skimmed through it, really like it! Thanks a lot!
Description
The thrilling sequel to #850 and probably the final part before @petar-qb does the actual implementation of this 🙌
Again I nominate @petar-qb as lead reviewer and @maxschulz-COL and second pair of eyes 🙏
Most changes here are to
_action_utils.py
, which was ready for a good refactoring._multi_load
indata_manager
. For some cases this could actually make a big difference to performance when caching is off (as is the default)Filter.pre_build
now also uses this_multi_load
functionalityaction_utils.py
. There is a clearer separation of responsibilities between different functions now.TODO NEXT
are short-term ones and will go once we've finished implementing dynamic filters@petar-qb I recommend reviewing as follows:
TODO:
Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":