Skip to content
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

Allow widget actions to propagate to children #1396

Merged
merged 4 commits into from
Jan 28, 2015
Merged

Conversation

ng110
Copy link
Contributor

@ng110 ng110 commented Jan 20, 2015

Modified invokeActions() in widget.js to recursively propagate action trigger event to all descendants. Maintains compatibility with call from button widget.

Modified select.js to add action triggering capability. Requires the widget.js modification to work.

test change
…rather than just immediate children.

Preserves compatibility with existing invokeActions call in button widget by creating a separate 'invokeActionCall' function to carry out the recursion.  Triggering all descendants permits use of action widgets inside list widgets or macros.  Also makes it possible to add triggering capability to select widget.
@tobibeer
Copy link
Contributor

@ng110 — don't exactly know why exactly but someone once quite emphasized never to pull on master #1371 (comment) ...so, I believe you're going to have to close this one and create a branch.

@tobibeer
Copy link
Contributor

hi @ng110, what I don't think your code handles is the case of there being html elements, so if you want that code to recursively traverse down the dom, you will have to wrap the recursion in a function and also call it on any html elements that act as intermediary wrappers, so as to check their children for action widgets

@Jermolene
Copy link
Owner

@ng110 it's OK to send a pull request on your master, but it means that you'll need to create a fork for any further unrelated modifications you might make.

There is still an unresolved problem with extending the select widget to invoke action widgets, which is that the there will not have been a refresh cycle to propogate the new value of the select widget. (I saw some discussion on another ticket or in the groups, but can't find the reference).

The best approach might be to separate the modification to the select widget into a separate pull request. I'll merge the propogation change and then we can address the select widget issue.

@ng110
Copy link
Contributor Author

ng110 commented Jan 27, 2015

Yes there was discussion on the refresh issue in the thread on the group that lead me into working on this. I didn't go on to look at that matter though because I solved my immediate problem with a bespoke widget, and life got in the way of doing more general purpose solutions.

Cheers,

Neil.

---- On Tue, 27 Jan 2015 13:07:39 +0000 Jeremy Ruston<notifications@github.com> wrote ----

@ng110 it's OK to send a pull request on your master, but it means that you'll need to create a fork for any further unrelated modifications you might make.
There is still an unresolved problem with extending the select widget to invoke action widgets, which is that the there will not have been a refresh cycle to propogate the new value of the select widget. (I saw some discussion on another ticket or in the groups, but can't find the reference).
The best approach might be to separate the modification to the select widget into a separate pull request. I'll merge the propogation change and then we can address the select widget issue.

Reply to this email directly or view it on GitHub.

@Jermolene
Copy link
Owner

@ng110 are you in a position to revise the pull request by removing the modification to the select widget? It'll leave a more confusing history if I merge the entire PR and then revoke that change.

@ng110
Copy link
Contributor Author

ng110 commented Jan 28, 2015

@Jermolene: I have committed a reverted select.js. Do I also need to update the pull request or is that automatic?

Would it be better if I also reverted the widget.js change and resubmitted on a separate branch?

N.

@Jermolene Jermolene changed the title modified widget.js and select.js Allow widget actions to propagate to children Jan 28, 2015
@Jermolene
Copy link
Owner

Hi @ng110 no, this is fine to merge.

Jermolene added a commit that referenced this pull request Jan 28, 2015
Allow widget actions to propagate to children
@Jermolene Jermolene merged commit 4cd8466 into Jermolene:master Jan 28, 2015
@tobibeer
Copy link
Contributor

Not sure if that would ever be needed, but what happens if the action widgets are not immediate children in the dom, but nested in some more html?

@Jermolene
Copy link
Owner

As long as they are descendents in the widget tree then they'll be triggered.

@tobibeer
Copy link
Contributor

Ok, thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants