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

fix(v-model): v-model ended with spaces #7735

Closed
wants to merge 6 commits into from

Conversation

Leocardoso94
Copy link

@Leocardoso94 Leocardoso94 commented Mar 2, 2018

fix #7730

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

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build-related changes
  • Other, please describe:

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

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

The PR fulfills these requirements:

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)

Other information:

@@ -69,6 +69,7 @@ type ModelParseResult = {

export function parseModel (val: string): ModelParseResult {
len = val.length
val = val.trim()
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be above the previous line 🤔

Copy link
Author

Choose a reason for hiding this comment

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

It's true, I fixed this in the last commit

@@ -19,6 +19,17 @@ describe('Directive v-model text', () => {
}).then(done)
})

it('should not create a new property ended with spaces when', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test that fails without the fix above please? This one passes
Also, could you rename the test title to should work with extra whitespace properties or something similar, please?

Copy link
Author

Choose a reason for hiding this comment

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

I did not understand the part of the test. Do I create another pull request with only the test? Because the current test fails without this fix

@posva
Copy link
Member

posva commented Mar 2, 2018

The current test does pass without the patch you added but it should fail. I'm asking if you could add a test case that fails without the val = val.trim() but passes with it

@Leocardoso94
Copy link
Author

Leocardoso94 commented Mar 3, 2018

Hi @posva , I add a e2e that fails without the val.trim()

Apparently, in the unit test, it was not possible to simulate this case, I believe it only creates the new property with extra spaces only when it is typed in the browser.

P.S. The extra property was only created if you pass a v-model from an object like v-model="obj.value "

@posva posva mentioned this pull request Mar 4, 2018
13 tasks
@posva
Copy link
Member

posva commented Mar 4, 2018

You needed to add a test with a nested object 😄
I added the test at #7737 and kept your initial commits

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.

Problem when a v-model end with spaces
2 participants