MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.
http://mudblazor.com
MIT License
7.21k stars 1.18k forks source link

Allow hex colors in place of Color enum #8783

Open danielchalmers opened 3 weeks ago

danielchalmers commented 3 weeks ago

Description

Adds a backwards-compatible way of using hex colors in places that accept the MudBlazor.Color enum by making it implicit with MudColor.

470

How Has This Been Tested?

Type of Changes

Compare the visuals to the code in the screenshot: image

Checklist

danielchalmers commented 3 weeks ago

How do I hook it up to the Theme Provider when MudColor doesn't accept CSS variables?

Do we need a separate class that can handle MudColor, Color enum, and CSS strings?

ScarletKuro commented 3 weeks ago

I find this switch very hard to read, especially if it will have more cases. Why can't we do this?

public static MudColor FromColor(Color color)
{
    var hex = color switch
    {
        Color.Primary => "#594AE2",
        Color.Secondary => Colors.Pink.Accent3,
        Color.Tertiary => "#1EC8A5",
        _ => "#594AE2"
    };

    return new MudColor(hex);
}

public static implicit operator MudColor(Color color) => FromColor(color);

Like why we want a constructor for Color? I feel like it should have an static function instead, considering it's exceptional usage, I don't find new MudColor(Color.Primary) to be useful, and you have an implicit converter which is doing a great job.

ScarletKuro commented 3 weeks ago

How do I hook it up to the Theme Provider when MudColor doesn't accept CSS variables?

Not sure I'm understanding the problem. Can you describe the problem in more detail with examples?

danielchalmers commented 3 weeks ago

@ScarletKuro the disgusting switch was just for quick demonstration :)

Can you describe the problem in more detail with examples?

You can define colors through the ThemeProvider and components will use them via CSS variables like --mud-palette-primary. How do we communicate that value through a MudColor backing field when the user passes MudBlazor.Color.Secondary through the parameter?

ScarletKuro commented 3 weeks ago

You can define colors through the ThemeProvider and components will use them via CSS variables like --mud-palette-primary. How do we communicate that value through a MudColor backing field when the user passes MudBlazor.Color.Secondary through the parameter?

Oh, now i get it. Yeah, that's pain

ScarletKuro commented 3 weeks ago

Off topic: If we will use MudColor everywhere for everything, we should consider making it a struct instead, it's a good candidate because it's already immutable so you don't get "evil" struct. Also we can make it fully span, but looking at the code there are plenty of nuances if we want to make it maximally optimized, would make the code less readable / maintainable.

ScarletKuro commented 2 weeks ago

Off topic: If we will use MudColor everywhere for everything, we should consider making it a struct instead, it's a good candidate because it's already immutable so you don't get "evil" struct. Also we can make it fully span, but looking at the code there are plenty of nuances if we want to make it maximally optimized, would make the code less readable / maintainable.

Actually, implementing this change isn't as straightforward as solving 2 + 2 (like I thought at first) While converting MudColor to a struct is very simple and its tests will pass, But it would require the overhaul ColorPicker, since structs are copied by value, there are a lot of cases when the picker will end up with 'stale' value as result many tests are red.