-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add post-add event to collection js #64
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks!
I'm curious, what do you need the event for?
And could you please add some tests for this change, the modifications have broken the current tests (while our travis builds look to be failing, there is output from the HHVM one: https://travis-ci.org/infinite-networks/InfiniteFormBundle/jobs/201476742#L343)
Resources/doc/collection-helper.md
Outdated
prototype will be the add button. In the case of the | ||
Polycollection, the prototype will be one of the prototype | ||
buttons. | ||
- `$row`: the jQuery wrapped DOM elements that was added to the collection. |
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.
grammar: dom elements that were added
Resources/public/js/collections.js
Outdated
if (event.insertBefore) { | ||
$row.insertBefore(event.insertBefore); | ||
this.$collection.trigger(addedEvent); |
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.
This can be moved below the if and before the return to avoid repeating the same call in both branches.
You should also only create the event before it is about to be fired, inside the isDefaultPrevented() check.
Hi Merk, Sure, I'll add the tests later today :) The reason we need the post-add event is for visual purposes really: items within the polycollection are styled in a way that allows them to act as a kind of editable preview, but we also allow the user to change the aspect ratio of the previews in a dropdown, so in the case that a user changes the resolution and then adds a new element, the prototype is unaware of the new resolution when it is created, so the event helps us hook into that and resize. If that makes sense? |
Hi Merk, looks like there's a failing php test on the 177.5 build only, even though this PR involves no php changes. Is this a known issue? |
The test failure is unrelated to your changes, I made some updates to fix our travis config and there is still a problem with PHP5.6 and our legacy symfony support for the Polycollection. |
No description provided.