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

Assertion to prevent setting both property and value on a form element #804

Merged
merged 2 commits into from
Apr 22, 2019
Merged

Assertion to prevent setting both property and value on a form element #804

merged 2 commits into from
Apr 22, 2019

Conversation

rrglomsk
Copy link
Contributor

The documentation does mention that the element's value will be bound if you use the property property, but we wound up with a few stray value properties set along with property on form elements in our application which lead to some unexpected bugs cropping up in production. It seems like a mistake that's not too difficult to make, so just an extra safety measure :)!

Copy link
Contributor

@jelhan jelhan left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@jelhan
Copy link
Contributor

jelhan commented Apr 17, 2019

Tests fails cause of a deprecation triggered in ember beta by carousel:

DEPRECATION: Please migrate from Ember._setComputedDecorator to Ember._setClassicDecorator [deprecation id: ember._setComputedDecorator]

Not related to this PR at all.

@simonihmig
Copy link
Contributor

Thanks @rrglomsk, that's a good improvement. Just a minor suggestion on my part...

Tests fails cause of a deprecation triggered in ember beta by carousel:

Yes, fixed by #802.

Co-Authored-By: rrglomsk <rrglomsk@users.noreply.github.com>
@simonihmig simonihmig merged commit 1eaf384 into ember-bootstrap:master Apr 22, 2019
@simonihmig
Copy link
Contributor

Thanks again!

@rrglomsk
Copy link
Contributor Author

No problem 😄!!

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