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 super calls inside paper-input lifecycle hooks. #506

Merged

Conversation

jordpo
Copy link
Contributor

@jordpo jordpo commented Sep 28, 2016

  • ensures hooks higher up in mixinx will fire properly

this was causing an error when we were using the paper-form input and dynamically adding/removing its child components in the form based on the current form data.

* ensures hooks higher up in mixinx will fire properly
@shoxter
Copy link
Contributor

shoxter commented Sep 29, 2016

@miguelcobain I was also experiencing this.

This should fix errors like Uncaught Error: Assertion Failed: calling set on destroyed object: <client@component:paper-input::ember1779>.isTouched = true

I haven't tested his fix, but I suspect this fixed the issue.

EDIT: I tested this and sure enough that was the issue.

@miguelcobain
Copy link
Collaborator

There is a form related error that is failing. Mind checking that?

@jordpo
Copy link
Contributor Author

jordpo commented Sep 29, 2016

@miguelcobain will do later today

* keeps super call on willDestroyElement fixing bug
* assumes the super call on didRender was causing issues on
non-default builds.
@jordpo
Copy link
Contributor Author

jordpo commented Sep 29, 2016

the only thing I can think of is the super call inside the didRender hook was screwing up the onValidityChange action from getting properly fired.

@shoxter
Copy link
Contributor

shoxter commented Sep 29, 2016

@jordpo @miguelcobain The onValidityChange was firing 3 times (as opposed to the 2 expected). This is what your failed build reason was. The assert.expect(2) is the faulty part in my opinion. I would expect there to be 3 assertions (one for each input on render, and then a third when setting the errors array).

@jordpo
Copy link
Contributor Author

jordpo commented Sep 30, 2016

the default scenario fails if we update the assert.expect to 3. I wonder if we can just take the assert.expect out?

@shoxter
Copy link
Contributor

shoxter commented Sep 30, 2016

@jordpo I really have to wonder why the default scenario fails at 3. I can't think out why it would be anything but 3. If we remove the assert.expect(n) we'd essentially just have to remove the test.

Maybe @miguelcobain can rectify why the default build returns anything but 3 assertions?

@miguelcobain miguelcobain merged commit 1ed37ea into adopted-ember-addons:master Oct 3, 2016
@miguelcobain
Copy link
Collaborator

@jordpo thanks for the PR. @shoxter was right. The action should be triggered three times. The test was wrong.

@jordpo
Copy link
Contributor Author

jordpo commented Oct 3, 2016

@miguelcobain 👍

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