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

UI: cleanup magic for signal-mechanism #578

Merged
merged 10 commits into from
Jul 25, 2017

Conversation

klees
Copy link
Member

@klees klees commented Jul 18, 2017

I noticed the opportunity to cleanup some magic that was introduced with the signal-mechanism.

People writing Renderers will now have an easier job. Triggerable and Triggerer need to be JavaScriptBindable anyway, because they in fact use that functionality. Some internals of AbstractComponentRenderer that were exposed during the introduction of the signal-mechanism are back to being internal.

Copy link
Contributor

@Amstutz Amstutz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for those improvements! Maybe a short comment on why this approach is superior will help devs. in future (see comment).

$show = $modal->getShowSignal();
$close = $modal->getCloseSignal();
$js = $this->getJavascriptBinding();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe explain in the comment of this function why this pattern here:

$js = $this->getJavascriptBinding();
$js->addOnLoadCode("
 			$(document).on('{$show}', function() { il.UI.modal.showModal('{$id}', {$options}); return false; });
 			$(document).on('{$close}', function() { il.UI.modal.closeModal('{$id}'); return false; });"
);

Is bad and should not be used and refer to this pattern:

return $modal->withAdditionalOnLoadCode(function($id) use (...) {
			return ...
});

To be used instead.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There you go.

Copy link
Contributor

@wanze wanze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the cleanup. One thing I'm not sure of: Now it is no longer possible for a renderer to get a unique ID without binding some javascript. In my opinion, this possibility should still exist. A component may want to easily identify another part in the DOM (part of its template) via ID.

@klees
Copy link
Member Author

klees commented Jul 20, 2017

@wanze: lets wait for the use case =)

@klees
Copy link
Member Author

klees commented Jul 21, 2017

@alex40724 Wanna have a look into this? I messed around (a little) with you drop down. If not, I'll merge.

@wanze
Copy link
Contributor

wanze commented Jul 21, 2017

@klees Actually I have a use case for the upcoming dropzones, but one can easily work around it ;-) Just to explain what I mean: Dropzones use a container to display the dropped files. Depending of the type of a dropzone, this container may be below the dropzone or in a modal. If it is possible to create an ID for this container, I can directly reference it in JQuery via:

var $fileListContainer = $('#myGeneratedIdForTheContainer');

Otherwise, I need to find my container with the knowledge where it exists in the DOM, e.g.

var $fileListContainer = $('#myDropzone').nextAll('.il-upload-file-list');
// or...
var $fileListContainer = $('#myDropzone').next('.il-modal-roundtrip').find('.il-upload-file-list');

Thus, if someone decides to move the file listing container in the dropzone template (e.g. display it above the dropzone), JS will break because it won't find the container anymore. I would prefer to have an ID to identify a component directly.

@klees
Copy link
Member Author

klees commented Jul 21, 2017

@wanze I bet you a beer that even in this case you do not need to generate an id via createId or similar and still won't need to rely on some relative position of your element in the dom. I mean, you need to put your js code var $fileListContainer = $('#myGeneratedIdForTheContainer'); somewhere where the id is known. There are two possible locations for this atm, these are inside withOnLoadCode and in your renderer after bindJavaScript. createId in that regard is only a little better then bindJavaScript. The only benefit it brings is that you could have an id in your renderer before bindJavaScript is called, but I don't see how this could help, because bindJavaScript only should have little dependency on other rendering code. There might be some cases with mutual recursion of ids (i.e. a needs to know b and b needs to know a), which you still should be able to easily untangle on js side. Then you still would need to put your js-code somewhere...
I'm not totally opposing createId, but for the framework part of the UI service I would prefer having as little principles as feasible. Feel free to ask me about your special case once you have some code ready.

@Amstutz Amstutz merged commit 91abdff into ILIAS-eLearning:trunk Jul 25, 2017
@klees klees mentioned this pull request Jul 28, 2017
srgitlab pushed a commit to srsolutionsag/ILIAS that referenced this pull request Aug 4, 2017
@klees klees deleted the trunk_ui_cleanup_binding_magic branch December 15, 2017 12:50
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.

3 participants