refactor: replace color getters and setters with mixin

[devversion]

• Replaces the color getters and setters with a Mixin function (similar as for the disabled property)
• This should reduce payload and also should make it way easier to maintain the color functionality.

Now it would be cool to say the Payload Diff github status 😉

[jelbourn]

CanColor (I want to consistently start all mixins with Can)

[devversion]

Sounds a bit weird, but makes sense to have it consistent

[jelbourn]

I'd call this HasRenderer

[devversion]

Should this still live in the color.ts file then?

[jelbourn]

Rather than having a run-time check, use string literal values for the type so that it's enforced at compile time:

protected _color: 'primary' | 'accent' | 'warn' | undefined = undefined;

[devversion]

Good idea with the types for TypeScript. Although I wanted to do the runtime check because people can still set the color to whatever they want and the class would be added like mat-test.

But yeah maybe this doesn't really matter since we enforce it using TypeScript. What do you think?

[jelbourn]

I don't think this method is really necessary; the setter can directly do

set color(value: string) {
  this._renderer.removeClass(this._elementRef.nativeElement, `mat-${this._color}`);
  this._renderer.addClass(this._elementRef.nativeElement, `mat-${value}`);
  this._color = value;
}

[devversion]

This doesn't work properly. Imagine someone sets the value to null. A class mat-null would be added.

For sure you can place the check if the class truthy inside of the getter too, but it feels better to do it like that.

[jelbourn]

Ah, I forgot the check; still think you can do it in the setter directly, though

[devversion]

Yeah true, but I mean it's just easier to read like that.

[jelbourn]

The main reason I don't like it is the isAdd argument:
https://martinfowler.com/bliki/FlagArgument.html

So my thinking is "How do we avoid the boolean argument? Well, it would just be two different methods. Oh, well we already have that on renderer, so just use that."

[devversion]

I agree that this might be an argument to move this to the setter itself but I kinda don't like having another level of nesting here:

set color(value: ThemePalette) {
  if (value || allowNoColor && !value) {
    if (this._color) {
       this._renderer.removeClass(this._elementRef.nativeElement, `mat-${this._color}`);
     }
     if (value) {
       this._renderer.addClass(this._elementRef.nativeElement, `mat-${value}`);
     }
     this._color = value;
   }
}

Any other ideas for this? I'm willing to change it though.

[jelbourn]

That seems fine to me; sometimes that's just how the logic goes.

[jelbourn]

Should do this with properties rather than a constructor because the constructor approach will generate more code.

[devversion]

I had to go back to this approach because just setting the properties inside of TypeScript means that the superclass cannot set the default color on initialization (maybe a trade-off)

[jelbourn]

That seems fine to me; sometimes that's just how the logic goes.

[jelbourn]

What is allowNoColor? I didn't notice that before

[devversion]

Some components allow having no color. E.g buttons don't have by default any color of primary, accent and warn.

[jelbourn]

Ah, I see it more as being some components require a color. Can we make it so that the argument is instead an optional defaultColor?

[devversion]

You mean that we can pass a value that is the default value to the mixin? (e.g null)

I'd rather do it the other way around because the default value "should" be basically everything that is falsy.

And if colors are required and someone sets it to a fasly value it should not change at all.

[jelbourn]

I mean

export function mixinColor<T extends Constructor<HasRenderer>>(base: T, defaultColor?: ThemePalette)
    : Constructor<CanColor> & T {
  return class extends base {
    private _color: ThemePalette = defaultColor;

    get color(): ThemePalette { return this._color; }
    set color(value: ThemePalette) {
      const newPalette = value || defaultColor;
      this._updatePaletteClasses(newPalette);
      this._color = newPalette;
    }
    
    constructor(...args: any[]) { 
      super(...args);
      if (defaultColor) {
        this._addPaletteClass(defaultColor);
      } 
    }
    
    private _updatePaletteClasses(newPalette: ThemePalette) {
      if (newPalette != this.color) {
        this._renderer.removeClass(this._elementRef.nativeElement, `mat-${this._color}`);
        this._renderer.addClass(this._elementRef.nativeElement, `mat-${newPalette}`);
      }
    }
  };
}

[devversion]

Ah I think I now know what you mean. This seems to be a good idea.

[devversion]

Made this change.

[jelbourn]

LGTM

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}