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

Allow hex colors in place of Color enum #8783

Draft
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

danielchalmers
Copy link
Contributor

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

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Compare the visuals to the code in the screenshot:
image

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added the enhancement New feature or request label Apr 22, 2024
@danielchalmers
Copy link
Contributor Author

danielchalmers commented Apr 23, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

danielchalmers commented Apr 23, 2024

@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
Copy link
Member

ScarletKuro commented Apr 23, 2024

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
Copy link
Member

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
Copy link
Member

ScarletKuro commented Apr 27, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants