Remove leftover two-way binding of form element's @value argument
#1097
Conversation
There was a problem hiding this comment.
Looks good to me. Thanks for providing this pull request.
@simonihmig Is dropping support for two-way binding of @value on <BsForm::Element> fine with you? This is also a prerequirement to rewrite it as a glimmer component.
|
|
||
| set value(value) { | ||
| this._value = value; | ||
| } |
There was a problem hiding this comment.
Wondering if Ember Bootstrap sets value itself or if the setter is only needed to support @value argument until the component is refactored to @glimmer/component. In the latter case I would move the assertion that not @value and @property could not be set at the same time to this setter. It would make it a little bit easier to follow the code in my opinion.
There was a problem hiding this comment.
It's the latter, and yes, I agree.
There was a problem hiding this comment.
Thanks @basz, the changes look fine. If you are able to address the little suggestion, that would be awesome!
Is dropping support for two-way binding of @value on BsForm::Element fine with you?
Absolutely! Tbh I wasn't aware we still had this around, as I never really used @value myself. This looks like a stupid left over from "ancient" times where we still had two-way bindings...
|
|
||
| set value(value) { | ||
| this._value = value; | ||
| } |
There was a problem hiding this comment.
It's the latter, and yes, I agree.
simonihmig
changed the title
@value argument
There was a problem hiding this comment.
Just noticed that documentation needs to be updated as well. At least the API docs for @value as they explicitly state that the value is also set.
The value of the control element is bound to this property. You can bind it to some controller property to get/set the control element's value:
It might be helpful to explicitly state that it's a one-way binding cause the combination of @model and @property are essentially a two-way binding.
|
@jelhan I'm not sure what you want the PI docs to say here... I have noticed it's two way bounded when an <Input @value=.../> is used... Which is by default for regular types... (text, email, etc). That is something the build-in input component does. I've pulled my hair a few times before I realised that. That thing is not Data Down Actions Up atm... |
I'm not sure what you are referring to. As far as I'm aware we don't use To be honest I think |
|
No we don’t. Sorry for that... |
|
Agree that should be deprecated or refactored |
|
Can you write that api doc? I’ll put it in the PR |
|
I'll merge this now, and file a API docs PR separately. Thanks @basz for your work here! |
I've noticed some issues when I use form in combination with models wrapped in ember-changeset changesets.
When I have a Changeset that has a model of
And a custom element that calls this.onUpdate(newValue) the changes appear to propagate into the changeset correctly, (changeset.changes has the changes) but unfortunately the new value is not transmitted back into the control on subsequent edits of the value. (It works only once)
You can see this behavior in ember.bushbaby.nl...
After much trial and error I have found out the element creates a dynamic property and aliases it to
model.${this.property}and that seems to be the culprit. I think it is the alias that does not work as reliably...I have created this PR to avoid dynamic assignment and aliasing. In my code base, my forms now work as expected... It is my hope it will make the 4.0 release.
also see #1089 for some discussion about two-way binding.