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

Add support for v-bind:content shorthand (fixes #2843) #2858

Closed
wants to merge 1 commit into from
Closed

Add support for v-bind:content shorthand (fixes #2843) #2858

wants to merge 1 commit into from

Conversation

phanan
Copy link
Member

@phanan phanan commented May 12, 2016

I personally quite like the feature request #2843, so I went ahead and try to support it.

With this commit, v-bind with an empty expression will imply the expression to be the directive name, i.e. v-bind :content will have the same effect as v-bind:content="content", :href will have the same effect as :href="href" and so on so forth. Note that only attribute bindings are affected by this change. Event, transition, class/style bindings etc. remain untouched.

Not sure if I'm missing anything here – all unit tests passed fine, though. Let me know if there's anything to fix or improve, or just close this PR if you don't intend to support the feature.

@bopjesvla
Copy link

Nice! Something I just thought of: v-bind:data-content and v-bind:ns:attr should be equivalent to v-bind:data-content="dataContent" and v-bind:ns:attr="nsAttr", respectively. I can submit a PR to your PR for that.

A second thing, which may be out of the scope of this PR: it'd be nice if custom directives had the ability to enable this behaviour.

@phanan
Copy link
Member Author

phanan commented May 12, 2016

v-bind:data-content and v-bind:ns:attr should be equivalent to v-bind:data-content="dataContent" and v-bind:ns:attr="nsAttr"

You're right, I overlooked these (especially the first part). Happy to receive your PR. @yyx990803 may or may not approve this feature btw.

This commit aims to add support for `v-bind:content` shorthand. With
this, v-bind with an empty expression will imply the expression to be
the directive name, i.e. `v-bind :content` will have the same effect as
`v-bind:content="content"`, `:href` will have the same effect as
`:href="href"` and so on so forth.

Only attribute bindings are affected by this change. Event, transition,
class/style bindings remain unchanged.
@@ -716,6 +717,11 @@ function compileDirectives (attrs, options) {
pushDir(dirName, internalDirectives[dirName])
} else {
arg = dirName
// support for shorthand expression (#2843)
// <div v-bind:content> => <div v-bind:content="content">
if (!value) {
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure about this? We don't want this to happen when the value is null or undefined?

Choose a reason for hiding this comment

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

I'm not sure if this is what you mean, but value is the expression, not the return value of the expression. So you can still do v-bind:content="null".

Copy link
Member

Choose a reason for hiding this comment

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

You're right, that's the rawValue. My bad!

@rpkilby
Copy link

rpkilby commented May 13, 2016

v-bind:ns:attr="nsAttr"

Is namespacing with colons an accepted syntax with Vue? Not sure if that would be accepted.

@LinusBorg
Copy link
Member

@phanan Should we close this PR in favour of #2877 ?

@phanan
Copy link
Member Author

phanan commented Jun 1, 2016

Sure. I don't think @yyx990803 is a fan of this change, though.

@phanan phanan closed this Jun 1, 2016
@phanan phanan deleted the vbind-shorthand branch June 1, 2016 14:37
@LinusBorg
Copy link
Member

All the more reason to not have two PR's about it :-P

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.

5 participants