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

Attach multiple decorators #399

Closed
browniefed opened this issue Jan 19, 2014 · 48 comments
Closed

Attach multiple decorators #399

browniefed opened this issue Jan 19, 2014 · 48 comments

Comments

@browniefed
Copy link

I was under the assumption that you could attach more than one decorator.
We are attempting to have 2 decorators. One for editing, and one for validating.

Although it appears that you can only have one decorator per element.
So I guess this is a feature request to have more than one decorator per element.

http://jsfiddle.net/M3MkH/

@cfenzo
Copy link

cfenzo commented Feb 10, 2014

I vote for this feature too (I also assumed you could have multiple decorators per element) :)

@MartinKolarik
Copy link
Member

I agree with @browniefed as well, but I think the syntax should be different. It's strange to have 2 decorator attributes.

@browniefed
Copy link
Author

@MartinKolarik Yeah I think having multiple decorator attributes but you are passing in data and other attributes of various formats you can't exactly comma delimit them.
Maybe if more than one decorator we declare them as decorator-nameofdecorator="valuesgettingpassedin"

@MartinKolarik
Copy link
Member

@browniefed I was wondering about decorator="{'name1': ['a', 'b'], 'name2': ['a', 'b']}"

@Rich-Harris
Copy link
Member

This is an interesting thread. I agree that this would probably be a worthwhile feature. The syntax question is tricky, and I don't think there's a perfect solution, but one thing that I think is important is that decorators should be ordered. It's not hard to imagine a situation where decorator x has to initialise before decorator y, because if decorator y goes first it will mutate the DOM in such a way that x breaks.

For that reason I'm a bit hesitant about solutions that use object notation or attribute names, since these are (conceptually at least) unordered lists. Maybe that's just my implementer's hat obscuring my vision, but it feels like a bit of a red flag (attributes could be forced to maintain their order with a bit of parser re-engineering, but it would be trickier with object notation). For that reason, my current inclination is towards semi-colons:

<div decorator='edit;validate'>edit decorator runs first, then decorator</div>

<!-- with parameters -->
<div decorator='edit:1,2,3;validate:{{foo}},{{bar}},{{baz}}'>edit decorator runs first, then decorator</div>

Thoughts?

@browniefed
Copy link
Author

What happens in the event that a simple string getting passed in contains a semicolon? That's why I'm always hesitant to use basic delimiters and even complex delimiters.
Edit: Nevermind saw the : separating the names, I'm okay with this method

@MartinKolarik
Copy link
Member

but one thing that I think is important is that decorators should be ordered

@Rich-Harris Good point!

I've been considering semicolons as well, but objects felt more natural and just like @browniefed I'm not sure what would happen if there was a semicolon in the parameter.

@Rich-Harris
Copy link
Member

The parser would have to accommodate semicolons, it wouldn't be a simple string.split(';') or anything - basically the same as if you had <div title='dogs > cats'>. A nuisance to implement but so be it!

The question is whether edit:foo;validate:bar should be allowed, or whether it should be edit:"foo";validate:"bar". At the moment, the algorithm is pretty basic - Ractive tries to use this module to parse parameters, and if that throws an error it falls back to assuming the parameter is one big string - that approach would no longer work.

@browniefed
Copy link
Author

If that's the case then I'm happy with it. If somehow someones data happens to be setup so perfectly that the parser identifies something as a decorator then so be it. Same sacrifices being made in that you couldn't technically observer on the key * since this would register as a pattern observer thus just don't use an asterisk as a key for your data.

@MartinKolarik
Copy link
Member

My third idea was decorator='[{name1: [1, 2, 3]}, {name1: [1, 2, 3]}]' It's logical and easy to parse, but a bit more complex than with semicolons.

@martypdx
Copy link
Contributor

one thing that I think is important is that decorators should be ordered. It's not hard to imagine a situation where decorator x has to initialise before decorator y, because if decorator y goes first it will mutate the DOM in such a way that x breaks

IMO, two order dependent decorators should be wrapped under a single decorator. Don't design the syntax around that. It's beginning to look like a JSON DSL for javascript.

(And what about the decorator teardown on argument change, does the whole line of decorators need to be recreated?)

I'd rather see the decorator syntax become more simplified by doing something similar to events in making the attribute name more meaningful, e.g. de-[decorator-name-token]="{{argument}}".

My .02 from other side of things...

@codler
Copy link
Member

codler commented Feb 11, 2014

The question is whether edit:foo;validate:bar should be allowed, or whether it should be edit:"foo";validate:"bar". At the moment, the algorithm is pretty basic - Ractive tries to use this module to parse parameters, and if that throws an error it falls back to assuming the parameter is one big string - that approach would no longer work.

@Rich-Harris , I would say use same behavior as Events.

My third idea was decorator='[{name1: [1, 2, 3]}, {name1: [1, 2, 3]}]' It's logical and easy to parse, but a bit more complex than with semicolons.

@MartinKolarik , with that it wouldnt feel natural with the current template syntax, I feel it is like an exception. :P

Maybe we can have decorator-nameofdecorator="valuesgettingpassedin" as @browniefed suggested, but for each decorator we can set a priority in the js.

@Rich-Harris
Copy link
Member

@martypdx makes some good points. Particularly now that decorators get updated/reapplied when their arguments change (#429) there is scope for all kinds of weird and hard-to-debug behaviour.

The idea of combining decorators is a good one:

<div decorator='edit_and_validate:[1,2,3],[{{foo}},{{bar}},{{baz}}]'></div>
Ractive.decorators.edit_and_validate = function ( node, editParams, validateParams ) {
  var editDecorator = Ractive.decorators.edit.apply( this, editParams ),
      validateDecorator = Ractive.decorators.edit.apply( this, validateParams );

  return {
    teardown: function () {
      validateDecorator.teardown();
      editDecorator.teardown();
    }
  };
};

This could be modified to accommodate decorators that support the update() method (again, see #429), or could be abstracted out into a generic decorator combiner. It seems like a more robust approach, bypasses the syntax debate, and wouldn't require any code to be added to the library.

@codler Decorator directives do actually use the exact same process as on-[event] directives (and intro/outro) - at parse time, we determine if there are any parameters. If so, and they're not dynamic, they're stored as an array. If they are dynamic, at render time they are turned into a StringFragment, which has a toArgsList() method that turns the fragment into a list of parameters. When the fragment changes, it calls its bubble() method, which is how we're able to update/reapply decorators.

@cfenzo
Copy link

cfenzo commented Feb 17, 2014

Allthough combining decorators like in the post above could be nice, I can imagine a real hell when/if you got a large list of generic and reusable decorators to combine into various combinations of 2 to x number of decorators per element.. So at least this shouldn't be the only way to attach multiple decorators to an element.

I would prefer the decorator-name="args" syntax, but if that makes preserving the order a pita, either ; delimetered or decorator1:[arg1,arg2],decorator2:[1,2] would work out nice too..

@martypdx
Copy link
Contributor

@cfenzo I really like the decorator-name="args" syntax, maybe even shortened to de-name or something, and ultimately multiple should be allowed. I think they should stay declarative with no guarantee of order (that needs to be handled by the dev).

In the meantime, this will probably help 90% of people on this thread. Here is a generic multi-decorator.

Example template is:

<div decorator="colorize: 130,157,206">some content</div>
<div decorator="biggerize: 20">some content</div>
<div decorator="multi: { colorize: [130,157,206], biggerize: [24] }">some content</div>

First, I've notice a common pattern for updating decorators with no teardown, so I created this function:

function decorator(fn){
   return function(node){
        var args = Array.prototype.slice.call(arguments, 1, arguments.length)
        fn.apply(node, args)
        return {
            teardown: function(){},
            update: fn
        }
    } 
}

Which can be invoked like so:

//new keyword optional
var colorize = new decorator(function(r,g,b){
    var color = 'rgb(' + [r,g,b].join(',') + ')'
    this.style.color = color // this === node
})
var biggerize = decorator(function(pt){
    this.style.fontSize = pt + 'pt'
})

The multi decorator is like so:

function multi(decorators){
    return decorator(function(toCall){
        var node = this
        Object.keys(toCall).forEach(function(d){
            var args = [node].concat(toCall[d])
            decorators[d].apply(null, args)
        })
    })
}

I used this pattern in the template: decorator="multi: { colorize: [130,157,206], biggerize: [24] }", you could make it whatever you want.

And it gets passed the decorators like they get passed to Ractive:

    decorators: {
        multi: multi({
            colorize: colorize,
            biggerize: biggerize,})
    }

Check out full example here: http://jsfiddle.net/jWYv8/1/

I would change it to an array if we wanted to guarantee order and a few other details (needs to handle the update case)

Let me know if this is of interest and I'll set it up as a proper plugin.

@martypdx
Copy link
Contributor

This version handles updates and teardowns

function multi(wrapped){

    return function(node, toCall){
        var decorators = {}

        Object.keys(wrapped).forEach( function(d){
            var args = [node].concat(toCall[d])
            decorators[d] = wrapped[d].apply(null, args)
        })

        return {
            teardown: function(){
                Object.keys(decorators).forEach(function(d){ 
                    d.teardown() 
                })
            },
            update: function(toUpdate){
                Object.keys(decorators).forEach(function(d){
                    decorators[d].update.apply(node, toUpdate[d])
                })                   
            }
        }
    }
}

see: http://jsfiddle.net/jWYv8/3/

@cfenzo
Copy link

cfenzo commented Mar 4, 2014

Wow, @martypdx ! that's just beautiful! 😃

Beeing able to set the order of the decorators would be nice :)

@martypdx
Copy link
Contributor

martypdx commented Mar 4, 2014

Created a plug-in, demo at http://martypdx.github.io/ractive-decorators-helpers/

EDIT: I recommend using newer plugin mentioned below: https://github.com/JonDum/ractive-multi-decorator

@MartinKolarik
Copy link
Member

@martypdx the plugin is great!

@MartinKolarik
Copy link
Member

@martypdx how do I get a reference to Ractive inside of the decorator?

@martypdx
Copy link
Contributor

@MartinKolarik: For the .combine() helper, I updated the plug-in to include current ractive instance as this, verified by this test:

var decoratorThis

var ractive = new Ractive({
    el: 'qunit-fixture',
    template: '<p decorator="combined: decorate">',
    decorators: {
        combined: decorators.combine([
            { 
                decorate: function(){
                    decoratorThis = this
                    return { teardown: function(){} }
                }
            }])
    }
})

assert.equal(decoratorThis, ractive)

For the .create() helper, it's intentionally simplified so node is this, but as an undocumented way, you can call ._ractive on the node, so this._ractive.

@MartinKolarik
Copy link
Member

Thanks @martypdx, actually I modified your plugin a bit (and was about to post it when your comment appeared) so that:

  1. this always refers to Ractive
  2. the first argument refers to node
  3. you can combine more decorators together, but only those defined in the decorator attribute are called

@MartinKolarik
Copy link
Member

@martypdx seems you accidentally dropped a PDF file into /lib folder

@akhoury
Copy link

akhoury commented Nov 21, 2014

I might be missing a major point here, but that what I understand.

Each decorator "constructor" have access to its "parent" ractive-instance, which exposes all accessible decorators, using ractive_instance.decorators, why do we even need to manually combine in a single decorator before hand? is it because you would rather do that on init-time? is there a performance hit by doing this below?

This version, inspired from @martypdx , will use combined decorators out of the box, no need to pass a special "combined" decorator to each ractive instance, or the call a create() to get "normalized" decorator.

The usage in the template is still the same:

// some global decorator
Ractive.decorators.globaldecorator = function() {/* ... */ return { teardown: function( ) { }, update: function() { }; } };

var ractive = new Ractive({
    el: 'target',
    template: '<div decorator="multi:{ localdecorator: [ {{ dynamicArg1 }}, arg2, arg3 ], globaldecorator: {{singleArg}}, anotherdecorator: true } " ></div>',
    // combined with local decorators
    // no need to "pre-combine" anything
    decorators: {
        localdecorator: function() { /* .... */, return { teardown: function() {}; } },
        anotherdecorator: function() {/* ... */, return { teardown: function() { }, update: function() {} };  }
    }
});

The global multi decorator

Ractive.decorators.multi = function(node, args) {
        var decorators = {};

        Object.keys(args).forEach(function(name) {
            if (typeof this.decorators[name] === 'function') {
                decorators[name] = this.decorators[name].apply(this, [node].concat(args[name]));
            }
        }.bind(this));

        return {
            update: function(args) {
                Object.keys(args).forEach(function(name) {
                    if (decorators[name]) {
                        decorators[name].update.apply(this, [].concat(args[name]));
                    }
                }.bind(this));
            },
            teardown: function() {
                Object.keys(args).forEach(function(name) {
                    if (decorators[name]) {
                        decorators[name].teardown.apply(this);
                    }
                }.bind(this));
            }
        };
    };

usage

<div 
    id="myelement"
    decorator="multi:{ 
        localdecorator: [ {{ dynamicArg1 }}, arg2, arg3 ], 
        globaldecorator: {{singleArg}}, 
        anotherdecorator: true 
    }"
></div>

GIST

@martypdx
Copy link
Contributor

@akhoury 👍 I thought of this after I wrote it, but I haven't needed to combine decorators in a long while :)

@briancray
Copy link

Just searched for this after I ran into the same issue. Perhaps a format of decorator-name="options" and if no options are needed, a simple boolean attribute like, <input decorator-autofill> where name is the exported name of the decorator.

@akhoury
Copy link

akhoury commented Apr 14, 2015

I second @briancray 's suggestion - the decorator-{decoratorName} format matches the on-{eventName} convention.

@LeJared
Copy link

LeJared commented Aug 5, 2015

+1 for decorator-name="options"

@Madgvox
Copy link
Member

Madgvox commented Aug 6, 2015

Although this is a relatively old topic, I have to agree that this proposed change is at least worth discussing. Perhaps this can be readdressed after the 0.8 release.

@LeJared
Copy link

LeJared commented Aug 7, 2015

Until the use of multiple decorators is supported by Ractive natively, I've create a "Meta"-Decorator, for combining multiple decorators on one element.

In contrast to Ractive-decorator-helpers you can use my solution out of the box with your existing decorators, because you don't have to change the way how to create your decorators in JS (which I don't like about Ractive-decorator-helpers).

Usage

<div decorator="combine:{decorator1:[param1, param2], decorator2:param, decorator3:{{myValue}}}"> Text </div>

Nothing else to do!

Here it is:

/**
 * Meta-Decorator to use multiple decorators on a single HTML-element
 * usage: decorator="combine:{decorator1:[param1, param2], decorator2:param}"
 */
Ractive.decorators.combine = function (node, args) {
    var name, decorator;
    var decorators = {};

    args = _.clone(args, true);

    for (name in args) {
        if ((decorator = this.decorators[name])) {
            decorators[name] = decorator.apply(this, [node].concat(args[name]));
        }
    }

    return {
        teardown: function () {
            for (var name in decorators) {
                decorators[name].teardown.apply(this);
            }
        },
        update: function (args) {
            var name, decorator;
            for (name in decorators) {
                decorator = decorators[name];
                decorator.update && decorator.update.apply(this, [].concat(args[name]));
            }
        }
    };
};

EDIT / Bugfix: I've changed this line in the code above: var decorators = _.clone(args, true);
The code now depends on the lodash library. It's important, that the data object is not used itself but a deep-copy of it. If you don't use lodash you can change this line to use your favorite lib or your own clone-funktion. e.g. whith jQuery it would be: var decorators = $.extend(true, {}, args);

@heavyk
Copy link
Contributor

heavyk commented Aug 7, 2015

@LeJared brilliant!!

@akhoury
Copy link

akhoury commented Aug 14, 2015

@LeJared how is that different from the multi decorator mentioned a few comments above ? By @martypdx ?

@LeJared
Copy link

LeJared commented Aug 14, 2015

To use the multi decorate from above or Ractive-decorator-helpers you have to change the way you create and assign your decorators to your Ractive instance. I don't like that, because you can't use exiting decorators out of the box, but have to create special combined versions of your decorators, to use them togethe. This also means, You have to create as create a new "combined decorator" for every combination separately!

Using my solution, you only have to change the way you assign your decorators in then templates and you can use any combinations of any decorators on the fly by just adding them in the template.

Benefit: once, Ractive supports multiple decorators natively, you only have to change your templates (you'll have to do this anyway!) but you don't need to change your decorators them selfs.

BTW: I've fixed a bug in the code above.

@akhoury
Copy link

akhoury commented Aug 23, 2015

@LeJared

you have to change the way you create and assign your decorators to your Ractive instance

I guess the initial idea of @martypdx that was true, but see my comment above, which basically was a slight improvement, you don't have to pre-combine them
#399 (comment)

that's how you use it in the template - all other decorators remain the same, and you don't have to create a combination before hand.

<div 
    id="myelement"
    decorator="multi:{ 
        localdecorator: [ {{ dynamicArg1 }}, arg2, arg3 ], 
        globaldecorator: {{singleArg}}, 
        anotherdecorator: true 
    }"
></div>

@martypdx
Copy link
Contributor

Yeah, after I wrote that I realized that you could just look them up from global registry.

What I find interesting is that I have yet to need to double up decorators on an element.

@Madgvox
Copy link
Member

Madgvox commented Aug 23, 2015

What I find interesting is that I have yet to need to double up decorators on an element.

The only situation where I've needed to double up a decorator is for a 'truncate' decorator, where I also want to have a 'tooltip' decorator on the element. I didn't need to have a multi-decorator plugin, though. I just instantiated the tooltip decorator from the truncate decorator manually. I'm not sure if that strategy is generally applicable however.

@LeJared
Copy link

LeJared commented Aug 24, 2015

@martypdx & @akhoury: Sorry, I missed that. So then both solutions are doing essentially the same.

@martypdx: What do you mean by have yet to need to double up decorators on an element? (Sorry, I'm not native english speaker).

@martypdx
Copy link
Contributor

What do you mean by have yet to need to double up decorators on an element?

@LeJared: I haven't needed a multi decorator.

@JonDum
Copy link

JonDum commented Sep 4, 2015

For ease of access, I slapped up @akhoury's solution as a repo and put it up on npm as ractive-multi-decorator

https://github.com/JonDum/ractive-multi-decorator

@martypdx
Copy link
Contributor

martypdx commented Sep 4, 2015

@JonDum 👍

Maybe it should be ractive-each-decorator:

<div 
    id="myelement"
    decorator="each:{ 
        localdecorator: [ {{ dynamicArg1 }}, arg2, arg3 ], 
        globaldecorator: {{singleArg}}, 
        anotherdecorator: true 
    }"
></div>

@zaus
Copy link

zaus commented Oct 19, 2015

Use case for multiple decorators: I have a list of items that I want to be able to both drag-drop AND click a button to add or remove items from the list. Would use:

@JonDum's repo seems to handle it.

@martypdx
Copy link
Contributor

@zaus I'm using @JonDum's repo too and works great.

@akhoury
Copy link

akhoury commented Jan 15, 2016

updated @JonDum's repo to avoid unnecessary updates on decorators which arguments haven't changed.

@akhoury
Copy link

akhoury commented Jan 25, 2016

hey @browniefed, may i ask why is this being closed? I thought it was labeled as a future enhancement.
Has it been decided that this feature won't be supported in core?

@browniefed browniefed reopened this Jan 25, 2016
@browniefed
Copy link
Author

@akhoury ah sorry I assumed it was already implemented for some reason haha, was cleaning up all my old open issues. Will keep thsi open

@evs-chris
Copy link
Contributor

This now possible in edge with multiple as-${decorator} directives 😄

@TehShrike
Copy link
Contributor

BOOYAH. What version will it be released in? Is Edge 0.8, 0.9, or even farther out?

@evs-chris
Copy link
Contributor

Edge is 0.8. I'm planning on cutting an alpha week after next.

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

No branches or pull requests