-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
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($vdom): Don't replace input for text-like type change, fix #6313 #6344
Conversation
* build(release weex): ignore the file path of entries * [release] weex-vue-framework@2.4.2-weex.1
@@ -48,14 +48,12 @@ function sameVnode (a, b) { | |||
) | |||
} | |||
|
|||
// Some browsers do not support dynamically changing type for <input> | |||
// so they need to be treated as different nodes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not relevant since it affects browsers that Vue doesn't support.
src/core/vdom/patch.js
Outdated
@@ -27,6 +25,8 @@ import { | |||
isPrimitive | |||
} from '../util/index' | |||
|
|||
const isTextInputType = makeMap(TEXT_INPUT_TYPES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could move function isTextInputType
to src/platforms/web/util/element.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javoski, done, any more feedback?
}).$mount() | ||
const node = vm.$el.children[0] | ||
vm.$el.children[0].value = 'test' | ||
vm.ok = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be vm.show? Same goes at 155.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😆, lol, that is a dumb mistake :D
waitForUpdate(() => { | ||
expect(vm.$el.children[0]).toBe(node) | ||
expect(vm.$el.children[0].value).toBe('test') | ||
vm.show = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also assert the input type attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dsonet, done, any more feedback?
I would suggest to skip warning in v-model if two changed types are all text-like. We can support full dynamic expression instead of only ternary expression. We can apply the logic in dom-props.js. (Also need to update |
@nickmessing I removed the ternary support parts to make this PR more focused. I have some ideas on how to fully support dynamic types on |
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
dev
branch for v2.x (or to a previous version branch), not themaster
branchfix #xxx[,#xxx]
, where "xxx" is the issue number)If adding a new feature, the PR's description includes:
Other information:
fix #6313