-
Notifications
You must be signed in to change notification settings - Fork 145
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] Proof of concept replacing ctd for filter and parameter #880
Conversation
View the example dashboards of the current commit live on PyCafe ☕ 🚀Updated on: 2024-11-15 15:26:40 UTC Link: vizro-core/examples/dev/ Link: vizro-core/examples/scratch_dev |
I like the initiative! There's a huge room for improvement regarding the In your code version, states are sent like:
It's similar to the current
The current filter_interaction way enables to propagate multiple different properties from of the same component to the server. So, we should preserve this flexibility I think. N.B. |
Great point, thanks @petar-qb. You're right that my proposed structure doesn't cater for one component with multiple After thinking about this more, I'm going to close this PR for now but open a ticket so we can revisit another time. Ultimately I'd like to try and handle controls using pattern-matching callback like this: plotly/dash#3063
and then do There's only two possible drawbacks with this I think:
As a first step we might adopt the same structure but without pattern-matching ids like this:
...but I'm not in a big hurry to do that because it will be heavier changes across the codebase and is not a priority. Let's try and do this only after we've removed There's not much point doing a solution like I suggested here with Let me know if it makes sense and I'll make a ticket for it and close this PR. |
Hey @antonymilne. Thanks for the great investigation!! Pattern-matching looks really promising, and from the top of my head it probably looks like the end goal in dealing with controls as actions inputs. (This solution looks like it could enable -> https://github.com/McK-Internal/vizro-internal/issues/576, but I can't guarantee that right now)
I totally agree with these two points. 👍 Let's create a ticket and deal with the |
Cool, thanks @petar-qb! Indeed this could solve a lot of things. e.g. also think of cases where a control could be added to a page dynamically. I don't think this would actually be possible without pattern-matching callbacks. I've made a new issue here: https://github.com/McK-Internal/vizro-internal/issues/1376. In due course I'll also make an issue to track forthcoming breaking changes to expect in 0.2.0, which currently exists just in rough notes on my computer. Once we've got the next couple of PRs on filters and actions done let's re-prioritise all the various actions v2 tickets because things have become a lot clearer to me over the last couple of months, and some tickets may no longer be relevant. |
Description
@petar-qb raising this PR just to see if you think it's a good idea as a change. I won't actually implement it until you've done your PR so you don't have to resolve any more conflicts.
I'm finally continuing work on #363 and this seems like a small refactor that will tidy things up some more before the conversion to classes, which will hopefully be the next PR I raise on this topic 🤞
So far this PR is just a proof of concept to show what the change would look like. I haven't fully rolled it out or updated tests yet but the simple demo app works exactly the same as before.
Basically we passed
filters
andparameters
throughinputs
in the callbacks before but didn't actually use them anywhere and instead looked insidectx.args_grouping
to extract all the values and and component ids. The only actual properties used in theCallbackTriggerDict
areid
andvalue
. We can instead pass through named states in the form{<id>: State(...)}
to get theid
passed in automatically so we don't needctx.args_grouping
for that. I don't think we'll ever need it for anything else. Here's what's in it to remind you:The change here would achieve be half of what you suggest in this comment:
The other half is also captured by my more recent comment:
Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":