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

Constant string in component HTML attributes #478

Closed
jccazeaux opened this issue Apr 26, 2015 · 16 comments
Closed

Constant string in component HTML attributes #478

jccazeaux opened this issue Apr 26, 2015 · 16 comments

Comments

@jccazeaux
Copy link
Contributor

In binders we can use simple quotes to define constant string

<div data-ad-my-binder="'Hello World !'"></div>

But in components this doesn't work, instead we must declare the attribute as "static". It would have been less confusing to have the same method no? Or there is a reason i don't see?

I would like to write

<my-component phrase="'Hello World !'"></my-component>

Instead of

rivets.components["my-component"] = {
    static: ["name"]
};
@jccazeaux
Copy link
Contributor Author

I have an other argument : when i create a component, i don't really know in advance in wich attributes i will use constant strings or model variables. This is especially true when i create a library of components for the developpers .
So it's hard to tell wich attributes i will have to declare static.

@stalniy
Copy link
Contributor

stalniy commented Jan 26, 2016

The best option here is to rewrite it to smth what Angular2 and Aurelia do.

I mean by default all attributes are static and when you suffix them with .bind they becomes dynamic (observable)

<tab title="test title"></tab>
<tab title.bind="article.title"></tab>

@blikblum
Copy link
Contributor

I agree that having the attributes be evaluated by default as static is better.
However, the syntax of the dynamic prop should be something like rv-bind-title="article.title" or rv-prop-title="article.title"
A little more verbose but consistent with rest of library

Another point is that this is backward incompatible so must be done with caution

@benadamstyles
Copy link
Collaborator

Rivets component attributes currently do not need the rv- prefix – I don't think this should be changed. The developer knows they are looking at a rivets component and the extra rv- is unnecessary. Also I think it would actually be a problem because I use rv- attributes in my components to refer to properties in the parent view – how would you separate these from the component attributes if they used rv- too? e.g.:

<my-component rv-if="config.shouldShowComponent" phrase="Hello World!"></my-component>

I use components extensively and roughly 90% of my component attributes are dynamic, not static. I am sure that this is why Mike originally wrote components so that the default is dynamic, not static.

I can see the value in declaring attributes as static on the component element, rather than in the component object, so I would suggest something like a static- prefix:

<my-component counter="model.counter" static-phrase="Hello World!"></my-component>

(a prefix, rather than a suffix, so that the actual name of the attribute is closest to its value)

If the default were static, developers would find themselves writing "bind" on almost every attribute, as the whole reason people use Rivets is to manage the properties of dynamic objects in the DOM.

@blikblum
Copy link
Contributor

@Leeds-eBooks

– how would you separate these from the component attributes if they used rv- too?

It would be a specific binding, e.g, rv-bind

Your example would be

<my-component rv-if="config.shouldShowComponent" rv-bind-phrase="config.phrase"></my-component>

As i said the problem with this is backwards compatibility.

To get backward compatible an alternative is use a binding for static props like you proposed. I would just add a prefix to get consistent with the rest of library:
<my-component counter="model.counter" rv-static-phrase="Hello World!"></my-component>

@jccazeaux
Copy link
Contributor Author

I agree, attributes must be dynamic by default. However, in dynamic attributes, we should be able to send a primitive data (string or int). It's possible in every binders:

<!-- Data from model -->
<div rv-mybinder="model.data"></div>
<!-- Data as primitive -->
<div rv-mybinder="'Hello !'"></div>

My point was : primitive values are not activated on components. To "simulate" primitive values we have to declare static attributes. If we activate primitive values it will be like all binders

Your exemple would like:

<my-component counter="model.counter" phrase="'Hello World!'"></my-component>

@benadamstyles
Copy link
Collaborator

@jccazeaux Yep I actually think your solution makes the most sense, probably, although the "' is not the prettiest! But if this is the standard rivets behaviour it makes sense to extend it to components. I think this would remove the need for the static property on the component object entirely, is that correct?

@jccazeaux
Copy link
Contributor Author

@Leeds-eBooks That's correct. I made the update on my github but never sent the pull request: https://github.com/jccazeaux/rivets/tree/Component-constant-attributes

@benadamstyles
Copy link
Collaborator

Well I'm personally in agreement if you did submit a pull request, but I don't think I should make this decision on my own. Would be good to get @Duder-onomy 's opinion on this at least, as well as the other people in this thread.

@blikblum
Copy link
Contributor

How about passing numeric values?

@benadamstyles
Copy link
Collaborator

As I understand it, you'd just do phrase="5". Although actually 5 could be a dynamic property name. So this wouldn't work?

@blikblum
Copy link
Contributor

I don't know, in the docs it says that primitive can be numeric, string or null but does not has examples with it.

Later i will do some tests and post here. I think we should replicate the behavior of binding syntax, if it accepts v-mybinder="'Hello !'" there, it should do to the component attributes as well, although the syntax is not pretty

@jccazeaux
Copy link
Contributor Author

Rivets handles numeric values as primitives. It's done in parsers.coffee:

 @parse: (string) ->
      if /^'.*'$|^".*"$/.test string
        type: @types.primitive
        value: string.slice 1, -1
      else if string is 'true'
        type: @types.primitive
        value: true
      else if string is 'false'
        type: @types.primitive
        value: false
      else if string is 'null'
        type: @types.primitive
        value: null
      else if string is 'undefined'
        type: @types.primitive
        value: undefined
      else if isNaN(Number(string)) is false
        type: @types.primitive
        value: Number string
      else
        type: @types.keypath
        value: string

As you can see, booleans, numbers, strings are considered as primitive types.

@stalniy
Copy link
Contributor

stalniy commented Jan 27, 2016

Ok, and what about id, class, title, hidden, other html specific attributes which expected to be static?

@benadamstyles
Copy link
Collaborator

As custom component elements are treated as divs by the browser, it is unlikely that any attributes other than class and id will be used. And given that the component element is really just a wrapper for the component template, and given that you can use the component name as a CSS selector, it is actually quite rare to use class and id on a component element. I have certainly never done it.

jccazeaux pushed a commit to jccazeaux/rivets that referenced this issue Feb 1, 2016
…. New tests for components reveal an error in forEach on attributes: attributes is not an array. An error was revealed too in View import : cyclic dependency between view and binding;
jccazeaux pushed a commit to jccazeaux/rivets that referenced this issue Feb 1, 2016
…. New tests for components reveal an error in forEach on attributes: attributes is not an array. An error was revealed too in View import : cyclic dependency between view and binding;
@stalniy
Copy link
Contributor

stalniy commented Feb 18, 2016

this may be closed

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

No branches or pull requests

4 participants