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

Bug with conditional attribute #2077

Closed
whitecolor opened this Issue Nov 12, 2015 · 9 comments

Comments

Projects
None yet
3 participants
@whitecolor
Contributor

whitecolor commented Nov 12, 2015

In 2.3 (it worked in 2.2):

{{#if preview}}next-page="{nextPage}"{{/if}}

If preview is true nextPage attribute value is string value {nextPage}, not actually value of nextPage in parent scope.

Maybe some why to do it with new binding sintax?

test is comming.

@justinbmeyer justinbmeyer added the bug label Nov 12, 2015

@justinbmeyer justinbmeyer added this to the 2.3.3 milestone Nov 12, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 12, 2015

Contributor

You tried this with master?

Contributor

justinbmeyer commented Nov 12, 2015

You tried this with master?

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Nov 13, 2015

Contributor

Yes with yesterday master.

Contributor

whitecolor commented Nov 13, 2015

Yes with yesterday master.

@justinbmeyer justinbmeyer self-assigned this Nov 13, 2015

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Nov 13, 2015

Contributor

In this case:

<some-comp {{#unless allowSome}}disabled{{/unless}}></some-comp>

if disabled attr is htmlbool in some-comp component, it even breaks with an erorr:

node_modules/can/view/stache/stache.js 301 Error: / is currently not supported within a tag.
Contributor

whitecolor commented Nov 13, 2015

In this case:

<some-comp {{#unless allowSome}}disabled{{/unless}}></some-comp>

if disabled attr is htmlbool in some-comp component, it even breaks with an erorr:

node_modules/can/view/stache/stache.js 301 Error: / is currently not supported within a tag.
@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 13, 2015

Contributor

@whitecolor I get no errors with:

test("conditional attributes (#2077)", function(){
    can.Component.extend({
        tag: 'some-comp',
        viewModel: {
            define: {
                disabled: {
                    type: 'htmlbool'
                }
            }
        }
    });
    var template = can.stache("<some-comp {{#unless allowSome}}disabled{{/unless}}></some-comp>");
    var map = new can.Map({allowSome: true});
    template(map);
    map.attr("allowSome", false);
    map.attr("allowSome", true);
});

Can you submit a test?

Contributor

justinbmeyer commented Nov 13, 2015

@whitecolor I get no errors with:

test("conditional attributes (#2077)", function(){
    can.Component.extend({
        tag: 'some-comp',
        viewModel: {
            define: {
                disabled: {
                    type: 'htmlbool'
                }
            }
        }
    });
    var template = can.stache("<some-comp {{#unless allowSome}}disabled{{/unless}}></some-comp>");
    var map = new can.Map({allowSome: true});
    template(map);
    map.attr("allowSome", false);
    map.attr("allowSome", true);
});

Can you submit a test?

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 13, 2015

Contributor

So the problem was that this was never working great to begin with. However in 2.0, it was working on component initialization because attribute callbacks were called before tag callbacks.

This meant that next-page="{nextPage}" was added to the element before the tag was created.

Now this happens after.

I'm trying to track down why we made this change, although I think it makes sense that components are invoked first.

Contributor

justinbmeyer commented Nov 13, 2015

So the problem was that this was never working great to begin with. However in 2.0, it was working on component initialization because attribute callbacks were called before tag callbacks.

This meant that next-page="{nextPage}" was added to the element before the tag was created.

Now this happens after.

I'm trying to track down why we made this change, although I think it makes sense that components are invoked first.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 13, 2015

Contributor

This commit changed each attribute to be added to the front: bdb06d6

This has the effect of making the tag run last because it will be added first.

This was added because bindings needed the view-model to be created before they ran. This is no longer the case because can-component sets up bindings itself.

So the hack solution is to reverse this. This will bring back compatibility.

But the better solution is to be able to handle attributes being added and removed.

The better solution isn't easy to write because binding split between components and can/view/bindings.

I might make this work with can/view/bindings without components first, and then make components do it and try to share as much code as possible.

Contributor

justinbmeyer commented Nov 13, 2015

This commit changed each attribute to be added to the front: bdb06d6

This has the effect of making the tag run last because it will be added first.

This was added because bindings needed the view-model to be created before they ran. This is no longer the case because can-component sets up bindings itself.

So the hack solution is to reverse this. This will bring back compatibility.

But the better solution is to be able to handle attributes being added and removed.

The better solution isn't easy to write because binding split between components and can/view/bindings.

I might make this work with can/view/bindings without components first, and then make components do it and try to share as much code as possible.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Nov 13, 2015

Contributor

@whitecolor to be clear ... the "good" short term fix for this is to have nextPage added at end of turn. This is slightly different then the behavior in 2.2. However, we can fix that later by having some sort of can.view.tagStart() and can.view.tagEnd(). tagStart will be called before any can.view.attr, and tagEnd will be called after all can.view.attr.

Having both will allow us to turn off all the view-binding attrs and then initialize the component once all the attributes have been written.

Contributor

justinbmeyer commented Nov 13, 2015

@whitecolor to be clear ... the "good" short term fix for this is to have nextPage added at end of turn. This is slightly different then the behavior in 2.2. However, we can fix that later by having some sort of can.view.tagStart() and can.view.tagEnd(). tagStart will be called before any can.view.attr, and tagEnd will be called after all can.view.attr.

Having both will allow us to turn off all the view-binding attrs and then initialize the component once all the attributes have been written.

@whitecolor

This comment has been minimized.

Show comment
Hide comment
@whitecolor

whitecolor Nov 14, 2015

Contributor

Ok thanks, for now I just got rid of such cases in the code. But it would be nice if it would start to work again.

Contributor

whitecolor commented Nov 14, 2015

Ok thanks, for now I just got rid of such cases in the code. But it would be nice if it would start to work again.

@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Nov 30, 2015

Contributor

Closed via #2089

Contributor

daffl commented Nov 30, 2015

Closed via #2089

@daffl daffl closed this Nov 30, 2015

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