adds custom attributes

[marclaval]

A first implementation of custom attributes.
To demo it, gestures have been made fully independent and optional module.

Before adding documentation and sample, a review is needed please.

[coveralls]

Coverage decreased (-0.2%) when pulling 40d3101 on mlaval:customAttributes into 679e110 on ariatemplates:master.

[coveralls]

Coverage decreased (-0.05%) when pulling eaf7bb6 on mlaval:customAttributes into 679e110 on ariatemplates:master.

[coveralls]

Coverage increased (+0.07%) when pulling 7439113 on mlaval:customAttributes into 679e110 on ariatemplates:master.

[PK1A]

Left some comments on individual commits. Generally it looks good to me but I'm afraid that I still don't have the full picture of the framework in my head to asses all the consequences of this change so it would be great to hear from @b-laporte

[divdavem]

If refreshAttributes is called several times (it is called each time an attributes changes), the same handler will be present several times in this._customAttributesHandlers and the cleanHandlers method will be called several times for the same node and same handler in $dispose...

[marclaval]

Yep, I realized that too and I'll improve it.

[divdavem]

As discussed with @Mlaval, I would prefer having a slightly different interface for custom attributes.
Instead of passing a handler object whose handleAttributes and cleanHandlers methods would be called, I would prefer being able to pass a constructor.
The constructor would be called (from createNode) to create an instance of the custom attribute for each node which uses the custom attribute. The setValue method of the custom attribute instance would be called each time the custom attribute value has changed. And the $dispose method would be called when the node is disposed. This way, it would be easier to track the life cycle of the node and to react to the changes of the attribute's value.

[marclaval]

Thanks for the comments @divdavem and @PK1A, I've amended the PR to do the changes, except for the API change suggestion that we should discuss first.

In addition, I've delayed the initial handling of custom attributes, it is now done after all node instances have been created. This way the handler can always access children and siblings.

[coveralls]

Coverage increased (+0.12%) when pulling 3f266f5 on mlaval:customAttributes into 679e110 on ariatemplates:master.

[coveralls]

Coverage increased (+0.14%) when pulling 0adb1ef on mlaval:customAttributes into 679e110 on ariatemplates:master.

[marclaval]

Finally implemented this feature as suggested by @divdavem , i.e. one instance of the handler is created per node where the custom attribute is added, and its lifecycle relies on $constructor, setValue and $dispose.
In addition:

• $constructor now receives a reference to the EltNode instance. This allows more powerful behavior in the handler.
• the registry can now accept a group of custom attributes with a single handler. In this case, only one instance of the handler is created for all the related attributes. This is a common use case and was needed for gestures.

On top of the test and the gesture refactor, I also challenged this code by implementing a dropdown custom attribute for https://github.com/ariatemplates/hashspace-bootstrap
A preview of the implementation can be seen here:
https://github.com/mlaval/hashspace-bootstrap/blob/dropdown/src/dropdown/dropdown.js
And its usage here:
https://github.com/mlaval/hashspace-bootstrap/blob/dropdown/demo/samples/dropdown/demo.hsp

It does work but it is limited and not very developer friendly. This raises questions about the API offered by the node instance, and the way attributes (class, model, ...) are managed within Hashspace.
Well, more reviews and discussions are needed.

[benouat]

@Mlaval @divdavem @PK1A
The feature looks good in terms of features, but I disagree on the Class structure that we have for the handler structure. We should not rely on the $klass utility for somekind of public API !!!

We should not force people to use it. They should be free to use whatever they want. For example, relying on $dispose is bad to me because it is specific to only us... What if people want to wrtite their code using pure ES6 modules ?

We should implement this feature with a dual approach:

• people declare a simple object that contains only generic methods (I don't know if it is feasable, but would be ideal).
• then, internally, we could use instance(s) of class(es) (using $klass) that we could create and mixin/overlay with their object(s).

It could be really much simpler, and should be more user friendly and generic.

[divdavem]

Copy/paste issue: the constructor is probably not used to dispose the handler instance.

[divdavem]

@benouat As far as I can see, the feature does not really rely on the klass utility.
It is possible to create a simple handler in pure javascript, for example:

var MyHandler = function (nodeInstance, callback) {
    // do something when the node is created (if needed)
};
MyHandler.prototype.setValue = function (name, value) {
    // do something when the value is set
};
MyHandler.prototype.$dispose = function () {
    // do something when the node is destroyed
};

And if the setValue or the $dispose methods are missing, they are simply not called without error.

[marclaval]

@divdavem : fixed the typo.
@benouat : changed a test to show how to use a simple handler as suggested by @divdavem . It will have to be documented correctly.

[benouat]

@divdavem @Mlaval fine that we can use a plain old his object, but I still would not consider publishing an api where some part rely on a private naming convention associated to a private tool! Even if we document it...

[divdavem]

@benouat Do you mean we should remove the $ from $dispose to make this public API cleaner?
Anyway, in my opinion we should be consistent with the $dispose method which can be defined in controllers (if we remove the $, we should remove it in both places...).

[benouat]

@divdavem you're right, I forget about these ones, but we might also change that.

We should maybe open another topic on that specific subject

[marclaval]

Added an independent handler for class attributes, which also helped improving the whole feature.
It updates class as safely as possible, i.e. trying to keep classes added externally (e.g. by another custom attribute) and to maintain the class order.

[marclaval]

Simplified a lot the class custom attributes as class order is not that important in an element, unless such rules are being written:

[class="class1 class2"] {
    background-color: yellow;
}

[class="class2 class1"] {
    border: 1px solid blue;
}

@benouat with the new code, using classList could remove one of the 2 split() left. I suggest to keep it simple for now.

[marclaval]

This PR also implements the refactoring that was discussed last week.

In terms of API, this PR introduces 2 new methods on the custom attribute handler:

• handleEvent(event): called by EltNode when an event listener is called at HTMLElement level.
• afterRefreshAttributes(): called by EltNode at the of the refreshAttributes process, needed because the handler can't know when attributes it handles have all been refreshed.

It also introduces a few methods on EltNode that enables a custom attribute to be powerful without knowing the internals of EltNode:

• setAttributeValueInDataModel(name, value)
• registerEventListeners(eventNames)
• getAncestorByCustomAttribute(name)
• getCustomAttributeHandlerInstances(name)

[coveralls]

Coverage increased (+0.22%) when pulling 2bf32ea on mlaval:customAttributes into a119d21 on ariatemplates:master.

[benouat]

I might be wrong, but I clearly don't see the class attribute as a custom one..
To me class attribute should be handled naturally with simply some | over the expression itself, and that's it.

Let's consider this sample

<div class="{ { 'class1': expression, 'class2': expression } | tokenList }"></div>

Where we could have this simple tokenList pipefilter, if the value is truthy, the key get appended as a css class

function tokenList(obj) {
    var tokens = [];
    for (var key in obj) {
        if (obj[key]) {
            tokens.push(key);
        }
    }
    return tokens.join(' ');
}

I don't see the custom behavior.
PS: It is actually done like that in polymer

Am I completely wrong on that one ?

[marclaval]

Well we could decide to implement it this way, it would work.

One drawback I see is that we'd loose one of the feature of the custom attributes: carefully updating the class attribute without overwriting it each time. This means that classes injected by others (e.g. a custom attribute) would be lost.
For example, the dropdown custom attribute implementation needs this feature as it adds and removes a open class to the node.