Thanood / aurelia-mdl-skeleton-navigation

A fork of aurelia-skeleton-navigation, using Material Design Lite.
MIT License
4 stars 2 forks source link

Setting `raised` on mdl-button causes error #1

Closed egeland closed 8 years ago

egeland commented 8 years ago

If I set raised on the button to false, I get errors:

This is my button: <mdl-button raised="false">Test</mdl-button> and my error is

ERROR [app-router] TypeError: Cannot read property 'classList' of null at MdlButton.raisedChanged (http://localhost:9000/dist/aurelia-mdl/mdl-button.js:101:32)

Other properties work ok, like <mdl-button primary="true">Test</mdl-button> gives me a primary-coloured button as expected..


After some investigation:

OK, so I think the issue is due to JS coercing - if I switch to using strings and strict equality, it works:

...
export class MdlButton {
    @bindable accent = 'false';
    @bindable colored = 'false';
    @bindable primary = 'false';
    @bindable raised = 'false';
    @bindable ripple = 'true';
    constructor(element) {
        this.element = element;
    }
    attached() {
        let btn = this.element.querySelector('button');
        if (this.accent === 'true') {
            btn.classList.add('mdl-button--accent');
        }
        if (this.colored === 'true') {
            btn.classList.add('mdl-button--colored');
        }
        if (this.primary === 'true') {
            btn.classList.add('mdl-button--primary');
        }
        if (this.raised === 'true') {
            btn.classList.add('mdl-button--raised');
        }
        if (this.ripple === 'true') {
            btn.classList.add('mdl-js-ripple-effect');
        }
        componentHandler.upgradeElement(btn);
    }
...

And the button <mdl-button raised="true">Test</mdl-button> now works!

Thanood commented 8 years ago

Thanks a lot for having a look at that. :) It seems "raised" is the only property having that problem because it's the only one with a "changed" handler.

When that handler executes the first time (and because the attribute is not bound: the only time), the <button> in the template does not exist yet.

I'm not sure if that's a bug or intentional. What do you think? Now looking at the handler, would "newValue" not always be truthy because it's "false" (as a string)?

Btw. if you use <mdl-button raised.bind="false"> it works.

Thanood commented 8 years ago

I'll merge it in. Let's see how it turns out. :)

Thanood commented 8 years ago

Closed since it fixes the problem. Thanks again. :)

I'm still not happy with the fact that the change event is fired before the inner button exists.. What's your opinion on that? If it's an Aurelia bug, we should probably post an issue but I'm not sure.

egeland commented 8 years ago

I didn't look at the newValue in the handler at all - but I guess it would need tweaking too, as you said, "false" (the string) is non-empty and therefore truthy..

Thanood commented 8 years ago

So, as the change handler doesn't make any sense at all when dealing with non-bound attributes (took me a while to get that :) ) I've just added a check to ignore it if the inner button doesn't exist.

For the other cases I've added a helper function getBooleanFromAttribute(attributeValue) to get what's needed.