-
-
Notifications
You must be signed in to change notification settings - Fork 132
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
Added ability to create a custom theme #69
base: master
Are you sure you want to change the base?
Conversation
Holy crap, awesome work, thanks! I'll check it out on Monday or Thursday, and approve it then. I don't remember if the backdrop should be same as the background, I'll take a look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took a look at your code, I don't like the circular dependency either, I also don't like that this is a breaking change (not TOO breaking, but I couldn't just bump the minor version for this one), Ideally we should come up with some way of keeping compatibility with existing form setups.
We could remove the circular dependency by keeping the ripple color "theme check" on the component and use the EnabledLightTheme
instead, this way the theme is independent of the ColorScheme. The other "black and white" ripples should be kept on the Theme.
As a side thought: in the future we could drop the concept of "light" and "dark" themes, as in two options, and make only "theme", we could auto-calculate if its dark or light or let the user tell us in a new property, but that's a story for another PR
ps: nice PR number 😏
@@ -440,8 +440,7 @@ private new void Paint(PaintEventArgs e) | |||
{ | |||
var rippleBrush = new SolidBrush(Color.FromArgb((int)(70 - (clickAnimProgress * 70)), | |||
UseColors ? SkinManager.ColorScheme.AccentColor : // Using colors | |||
SkinManager.Theme == MaterialSkinManager.Themes.LIGHT ? SkinManager.ColorScheme.PrimaryColor : // light theme | |||
SkinManager.ColorScheme.LightPrimaryColor)); // dark theme | |||
SkinManager.Theme.RippleColor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep this check, just use EnabledLightTheme
instead
SkinManager.Theme == MaterialSkinManager.Themes.LIGHT ? SkinManager.ColorScheme.PrimaryColor : // default light | ||
SkinManager.ColorScheme.LightPrimaryColor)); // default dark | ||
_backgroundWithAccent ? SkinManager.ColorScheme.AccentColor : // defaul accent | ||
SkinManager.Theme.RippleColor)); // default dark |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same, keep the check, use EnabledLightTheme
@@ -312,7 +312,7 @@ protected override void OnPaint(PaintEventArgs pevent) | |||
SkinManager.ColorScheme.AccentColor.Lighten(0.5f) : // Emphasis with accent | |||
SkinManager.ColorScheme.LightPrimaryColor) : // Emphasis | |||
(UseAccentColor ? SkinManager.ColorScheme.AccentColor : // Normal with accent | |||
SkinManager.Theme == MaterialSkinManager.Themes.LIGHT ? SkinManager.ColorScheme.PrimaryColor : SkinManager.ColorScheme.LightPrimaryColor))))) // Normal | |||
SkinManager.Theme.RippleColor))))) // Normal |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the check, use EnabledLightTheme
materialSkinManager.Theme = MaterialSkinManager.Themes.LIGHT; | ||
materialSkinManager.ColorScheme = new ColorScheme(Primary.Indigo500, Primary.Indigo700, Primary.Indigo100, Accent.Pink200, TextShade.WHITE); | ||
materialSkinManager.AddFormToManage(this); | ||
materialSkinManager.SetThemes(new ThemeLight(materialSkinManager), new ThemeDark(materialSkinManager), true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a breaking change, can we keep the MaterialSkinManager.Themes.LIGHT
somehow? Maybe use a static class? also please fix the tab spacing :)
materialSkinManager.Theme == MaterialSkinManager.Themes.DARK ? Primary.Teal700 : Primary.Indigo700, | ||
materialSkinManager.Theme == MaterialSkinManager.Themes.DARK ? Primary.Teal200 : Primary.Indigo100, | ||
!materialSkinManager.EnabledLightTheme ? Primary.Teal500 : Primary.Indigo500, | ||
!materialSkinManager.EnabledLightTheme ? Primary.Teal700 : Primary.Indigo700, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also a breaking change, we should come up with some way of keeping the check materialSkinManager.Theme == MaterialSkinManager.Themes.DARK
valid, maybe create a compatible static class?
{ | ||
materialSkinManager.Theme = materialSkinManager.Theme == MaterialSkinManager.Themes.DARK ? MaterialSkinManager.Themes.LIGHT : MaterialSkinManager.Themes.DARK; | ||
{ | ||
materialSkinManager.SwitchTheme(MaterialSkinManager.ThemeSelector.Opposite); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use a static class to keep compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see notes for changes
I would love to if the actual class just gave us the ability to provide I even do that one myself on one of my programs and expect things (up to and including TextBox's and everything to be dark, but I plan to make it per icon shown on all the program windows where the icon that is black triggers dark theme, the pink icon triggers pink theme, and the orange icon triggers orange theme). However doing it manually made it almost impossible because on |
Originally,
MaterialSkinManager
accepted either a light or dark theme, both being predefined. There was no option to customize the themes or have a different background color of the window (none that I'm aware of).I added the ability to customize an existing theme or create a new one, together with an easy mechanism for switching themes.
I'm not very content of the circular dependency:
but I added it to keep the existing behavior.
I was also wondering whether
BackdropBrush
is correctly set - in both cases it was set to a brush fromBackgroundColor
instead ofBackdropColor
. Let me know.Thanks