-
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
[Docs] Dynamic filters #891
Conversation
for more information, see https://pre-commit.ci
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 exciting! 💯 🚀 Amazing job you two @petar-qb @antonymilne!
Overall well written and clear, but one thing where I still have an open question is what happens with a page with two charts, drawn from different data sources, one dynamic and one static.
In general, do we have anywhere in the docs that the Filter takes the union of values, if the selected column is present in two data sources?
Thanks for the review @huong-li-nguyen, it's super helpful to get a fresh pair of eyes on this!
Hmm, good point - not really as far as I can see. On the filters docs page we say "By default, all components on a page with such a column present will be filtered. The selector type will be chosen automatically based on the target column, for example, a dropdown for categorical data, a range slider for numerical data, or a date picker for temporal data." But there's nothing explaining how the range/options itself are found from the union of values. Let me add this! |
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 from a docs perspective 🌟 🌟 🌟 🌟 🌟
I left a few comments and suggestions for tweaks but I'm super happy with this and don't need to see them again. Approving 🙏
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! Pretty cool functionality 🚀
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.
Great work! 🥇
Co-authored-by: Jo Stichbury <[email protected]>
Co-authored-by: Jo Stichbury <[email protected]>
Co-authored-by: Petar Pejovic <[email protected]>
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.
Thanks for addressing the comments. I reviewed it again and I think this PR is ready for merging.
The only remaining minor thing that maybe could be improved is swapping the following two sections:
"Override selector options to make a dynamic filter static"
and "Dynamic filter with specific selector is still dynamic"
. Just consider this and ignore if irrelevant 😃
@petar-qb I tried swapping those round and found it made things worse overall because the last section on "filters that depend on parametrized dynamic data loading" became less clear as it sounded like it worked with static data, which didn't make sense. And it also didn't make sense to move that section further up the page. @huong-li-nguyen added explanation of how filters choose |
Description
Docs for #879.
As discussed in #461, our current
load_iris_data
example is not very inspiring and we should change it or come up with a second more advanced example. For now I've kept it the same though. Let's update once we've got parametrised data filters fully working.Please review:
@petar-qb - have I missed anything?
@maxschulz-COL @huong-li-nguyen does it make sense? Some of it was hard to explain so would be great if someone who has not been so deep in the weeds on this could read through it.
@stichbury the usual 🙂
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":