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.

Sign up for GitHub

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

Jump to bottom

Allow v-bind="$attrs" with v-on="$listeners" to work with v-model #6327

Closed
wants to merge 5 commits into from
Closed

Allow v-bind="$attrs" with v-on="$listeners" to work with v-model #6327

wants to merge 5 commits into from

Conversation

chrisvfritz
Copy link
Contributor

What kind of change does this PR introduce? (check at least one)

  • Bugfix?
  • Feature

Does this PR introduce a breaking change? (check one)

  • No, unless someone was using v-model to purposefully bind an element's value directly to a DOM Event, which I think would be strange.

If adding a new feature, the PR's description includes:

  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

I brought this up in #6216 a couple weeks ago and it didn't encounter any opposition, so here we go!

TLDR: Since v-bind="$attrs" and v-on="$listeners" were added, a lot of people have been assuming that combining these should be sufficient to allow v-model on the component. This PR makes v-model work as users expect.

Hanks10100 and others added 2 commits July 24, 2017 07:32
@Hanks10100 @yyx990803
[release] weex-vue-framework@2.4.2-weex.1 (vuejs#6196)
c628103 
* build(release weex): ignore the file path of entries

* [release] weex-vue-framework@2.4.2-weex.1
@chrisvfritz
allow v-bind="$attrs" with v-on="$listeners" to work with v-model
1f89cc9
@chrisvfritz chrisvfritz force-pushed the feature/v-model-through-attrs-and-listeners branch from aecc154 to 1f89cc9 Compare August 8, 2017 20:12
@chrisvfritz chrisvfritz mentioned this pull request Aug 8, 2017
Closed
@chrisvfritz
Copy link
Contributor Author

I just noticed some Weex tests are failing. I could easily make them pass, but due to the nature of the tests and my own lack of experience with Weex, I'm not confident this would actually guarantee working behavior.

@Jinjiang Would you mind taking a look? Is it safe to update the tests, or is it possible my changes would break Weex?

@javoski
Copy link
Member

javoski commented Aug 9, 2017

Seems $event.target.value will break weex, browser-based code shouldn't exist in cross-platform part.

@chrisvfritz
Copy link
Contributor Author

chrisvfritz commented Aug 9, 2017 •
edited
Loading

@javoski Are you sure this would break Weex? I only try to access $event.target.value if the $event is an object and $event.type === 'input'. In cases where a DOM Event (or something resembling a DOM Event) isn't attached to the component event (which I think should always be the case in native environments), Weex would not see changed behavior.

@Jinjiang
Copy link
Member

Jinjiang commented Aug 9, 2017

@chrisvfritz sorry it's still loading status in "$ bash build/ci.sh" part of the ci report (maybe my network problem). I will have a look in the deep. Just a moment please :-)

@Jinjiang
Copy link
Member

Jinjiang commented Aug 9, 2017

@chrisvfritz This commit is fine. Currently you can just skip these failed test cases in weex. And @Hanks10100 and I will fix them later.
Thanks.

@Hanks10100
Copy link
Contributor

@chrisvfritz After I confirmed that this feature will not destroy weex, I will modify those failed test cases and add the corresponding case in Weex.

@Hanks10100
test(weex): add test case of v-model and v-bind="$attrs" and v-on="$l…
ff1a2c6 
…isteners"
@Hanks10100
fix(compiler): support to get the correct value of input element in Weex
13ec6d8 
In Weex, the structure of the `<input>` element has a slight difference with Web, it doesn't have
the `value` property but has `attr.value`. It should be considered when getting the value of input
element.
@Hanks10100 Hanks10100 mentioned this pull request Aug 10, 2017
Merged
@Hanks10100
Copy link
Contributor

@chrisvfritz I fixed the test case in Weex and send a PR (chrisvfritz#1) to your branch. The CI is passed.

Your modification didn't break Weex, but not consider the difference of it.

As I write in that PR:

In Weex, the structure of the <input> element has a slight difference with Web, it doesn't have the value property but has attr.value.

@chrisvfritz
Merge pull request #1 from Hanks10100/weex-feature-v-model
f188fa0 
Fix the test case in Weex and get the correct value of input element
@SergejKasper
Copy link

Can this be merged? It would be a great help for wrapping inputs!

@yyx990803
Copy link
Member

See #6216 (comment)

@yyx990803 yyx990803 closed this Oct 2, 2017
@chrisvfritz
Copy link
Contributor Author

Yeah, I agree it felt a little hacky and certainly didn't cover every use case. Part of me was hoping you'd think of some small refactor that would simplify things, but we weren't lucky this time. 🙂

@davidhewitt davidhewitt mentioned this pull request Jan 8, 2018
Closed
@lbogdan lbogdan mentioned this pull request Jan 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jinjiang Jinjiang Jinjiang approved these changes

@yyx990803 yyx990803 Awaiting requested review from yyx990803

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chrisvfritz @javoski @Jinjiang @Hanks10100 @SergejKasper @yyx990803