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

Fixed failing "sets the value prop if it was changed in the DOM" #418

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kay999
Copy link
Contributor

@kay999 kay999 commented Feb 7, 2019

This shows and fixes #417 (comment)

This shows that the current implementation fails if the DOM is changed directly (for example by user input). Because snabbdom doesn't see any changes in the vdom, it doesn't patches it.

@kay999
Copy link
Contributor Author

kay999 commented Feb 7, 2019

The CI server seems to have problems running testem in the moment, so the fix is marked as failed.

Copy link
Contributor

@mightyiam mightyiam left a comment

Choose a reason for hiding this comment

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

I'm surprised that this isn't working properly. I'd like to take a look at the blame for that line. For now, here's a possibly better test title?

@@ -264,6 +264,14 @@ describe('snabbdom', function() {
patch(vnode1, vnode2);
assert.equal(elm.src, undefined);
});
it('changes an elements value-prop it was changed in the dom', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('changes an elements value-prop it was changed in the dom', function() {
it('sets the `value` prop if it was changed in the DOM', function() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name change is ok.

I hope it won't take too long to get into a release, because it's a quite annoying problem if you want any kind of input-field completion/formatting.

One possible extension would be enabling similar behaviour for the 'selected' and 'checke' props. But those aren't as often overwritten as value so it's not as important.

But we may need a documentation update for this.

Choose a reason for hiding this comment

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

I would be in favor of doing this for selected and checked as well, because overwritting these does happen (at least in my application). Thus, it would be really convenient to solve these properties, too.

@kay999 kay999 changed the title Fixed #417 Fixed failing "sets the value prop if it was changed in the DOM" Feb 17, 2019
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.

Logical error in props module?
3 participants