-
Notifications
You must be signed in to change notification settings - Fork 327
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: Allow by-name component access. #148 #1870
Conversation
definitely yes imho |
Can you expand with some examples? |
I have no concrete example at hand, but I found these |
Oh, I should have made the question more clear then. Let me rephrase. The problem you just described is already implemented in this PR:
|
ah lol sry my bad.
That's nothing I find that useful as of now (no use case comes to mind). |
I mean there are certainly use-cases for some sort of batch processing of components like enabling/disabling or resetting, but at least personally I didn't miss that feature yet. |
Given two scripts S1 and S2, say:
The final caption will be Instead, say there was a way to name the item
For this to work, the server needs to be aware that |
More thoughts: this is tricky to do primarily because The other option is to merge your PR, but with the caveat that by-name access is not supported for scripts, or for the uni-/multicast apps. |
Thank you for working on this @mturoci . This will be super handy when updating single items on a page. I can't say anything about the technical implementation, but I am wondering if it works on any hierarchy level of the component? Does any process enforce name uniqueness? If not, how would that be resolved? Would that go into the direction of your second question?
|
I would like to avoid this if possible. Thanks for the pointers, will see what I can do.
It should. If you check my example above, I target a button what is within items > inline > button.
There is no enforcement from Wave side currently (#1493). The name uniqueness needs to be preserved only within the "card" scope. This means that Generally speaking, I think we should enforce unique names per card as it makes sense for getting the outputs via |
…dcast - needs discussion. #148
Seems like I managed to find the optimal solution to support scripts, broadcast, multicast as well without any serious perf degradation. In fact, it should run a bit faster now. @geomodular could you please review the Go code to make sure it's idiomatic? The parts I am not so sure about:
|
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.
Good job, Martin. Overall I think it's fine. Take my comments as suggestions.
Unsafe type casts.
This sometimes emerges. You have interface{}
; you need to cast. If by unsafe
you mean without the type checking, then I'd say it's better to check. But it depends on the context.
Is fillNameComponentMap function okayish since it mutates passed hashmap, thus is not a pure function.
That's ok. We are not even trying to do functional programming in go
. I'd say the function is readable, and that's the goal.
I believe map[string]interface{} only stores pointers to the actual objects rather than values. Is this correct?
Yes, that's correct. map[string]interface{}
is essentially a dictionary of pointers.
Thanks for the review @geomodular!
Yes, that's exactly what I meant, especially the All the comments were addressed. Updated the go version via |
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.
Marvelous!
This is a killer feature, can't wait to try it out. |
Available in nightly, feedback / bug reports encouraged since this spans all the components throughout Wave.
Glad to hear that! |
Allows for:
Needs discussion
@lo5 can you elaborate what you meant here? AFAIU, the whole UI state is stored client-side meaning, there is not much to be done at the server-side interpreter unless I am missing something. Apart from guarding the accessors.
Wondering whether these would be useful - cc @Far0n @pascal-pfeiffer @vopani @srini-x for feedback.
Closes #148