Skip to content
This repository has been archived by the owner on Dec 16, 2021. It is now read-only.

Add support for defaultValue to preact-compat #280

Merged
merged 5 commits into from
Jan 12, 2017

Conversation

ghostlandr
Copy link
Contributor

@ghostlandr ghostlandr commented Jan 9, 2017

Fixes #261

This adds support for this functionality which I missed from the first time I tried out preact-compat. Somehow I forgot that it was missing until I went to release my app to some beta users today and realized it was kinda broken as it is haha.

I worked on this with @developit so hopefully it doesn't need much massaging to merge 👍

@@ -15,6 +15,7 @@
"test": "npm-run-all lint build test:karma",
"lint": "eslint {src,test}",
"test:karma": "karma start --single-run",
"test:watch": "karma start --watch",
Copy link
Member

Choose a reason for hiding this comment

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

Just a note - I believe karma start watches by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I bet you're right. I'll adjust as needed.

@@ -82,14 +82,24 @@ options.vnode = vnode => {
}
}
else if (attrs) {
if (typeof vnode.nodeName==='string' && vnode.attributes && vnode.attributes.defaultValue) {
addDefaultValue(vnode);
Copy link
Member

Choose a reason for hiding this comment

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

@gholtslander-va I know we started out with this as a separate function, but I'm thinking since it ended up being pretty small (and not creating a closure) we should inline it for bundle size.

if (typeof vnode.nodeName==='string' && attrs && attrs.defaultValue) {
	if (!attrs.value && attrs.value!==0) {
		attrs.value = attrs.defaultValue;
	}
	delete attrs.defaultValue;
}

Seem reasonable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds perfect

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

Would love to get the function inlined before merge, let me know if that's cool with you!

Copy link
Member

@developit developit left a comment

Choose a reason for hiding this comment

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

Awesome! We can release this first thing in the morning :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants