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

[WIP] Pass affected elements as extra parameter in collection js events #380

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Maff-
Copy link
Contributor

@Maff- Maff- commented Jun 5, 2015

This PR is an expansion of issue #311

This small change allows you to use the JS events triggered in bc-bootstrap-collection.js more effectively, as it passes the affected list elements as extra parameter.

These parameters are available in the event listeners attached with jQuery's .on(). With this in place there's no need for 'guessing' which element was added. (e.g. getting the last child of the <ul>, which isn't necessarily the right element)

Example usage:

{% block javascripts %}
    {{- parent() }}
    <script>
        (function ($) {

            var $commentsCollection = $('{{ '#' ~ edit_form.comments.vars.id }}');

            $commentsCollection
                    .on('bc-collection-field-added', function (e, $item, $collection) {
                        console.log('comment form added to collection: ', $item);
                        $item.find(':input:first').focus();
                    })
                    .on('bc-collection-field-removed', function (e, $item) {
                        console.log('comment form removed from collection: ', $item);
                        // do nothing :)

                        // note: it's a bit weird the event gets fired _before_ the element is removed,
                        // but if we trigger if after the remove(), this event will not be caught.
                    })
            ;

        })(jQuery);
    </script>
{% endblock %}

The changes here should be backwards compatible, and this PR could be merged right away, but I would like to discuss something here:

I see still some issues with the current script.
Firstly, the events are triggered on the add and removed buttons, rather that on the collection (div) for example. Imho this is a bit weird, and causes a problem if we would change the moment the removed event is triggered.
Which brings me to issue two; the bc-collection-field-removed event is triggered before the element is removed. We could swap this around, but this won't work with listeners attached to the collection (like in the example above), as the removed <li> has no ancestors to bubble the event to.

My suggestion would be to use the collection element (the <div>) as the attached element, and use the parameters (like done in this commit) to pass the relevant <li>. We can than trigger the deleted event after the element is removed.

As this suggestion would cause breaking changes I would love some feedback on this.

@versh23
Copy link

versh23 commented Oct 12, 2015

+1

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

Successfully merging this pull request may close these issues.

2 participants