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

Add post-add event to collection js #64

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add post-add event to collection js #64

wants to merge 6 commits into from

Conversation

ste-wilkinson
Copy link

No description provided.

Copy link
Contributor

@merk merk left a 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)

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.
Copy link
Contributor

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

if (event.insertBefore) {
$row.insertBefore(event.insertBefore);
this.$collection.trigger(addedEvent);
Copy link
Contributor

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.

@ste-wilkinson
Copy link
Author

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?

@ste-wilkinson
Copy link
Author

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?

@merk
Copy link
Contributor

merk commented Feb 22, 2017

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.

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.

3 participants