MaterialDesignInXamlToolkit icon indicating copy to clipboard operation
MaterialDesignInXamlToolkit copied to clipboard

Paper Variant Group Boxes

Open rrs opened this issue 3 years ago • 7 comments

First this is not ready to go as is, and needs some input. It is to complement the paper variant buttons added previously. The goal as with the paper buttons is to chill out the theming on the group boxes so they are less imposing. I have had a play with a few different types

groupboxes

4 different attempts.

They all use the paper background, so they would always be dark or light even on colored backgrounds. Starting with r1c3 (row/col) - This is the current 'default' which has shadow 1, PrimaryHueMid for the line and weirdly MaterialDesignCardBackground for the border, just to try and lift the contrast on it, but more on that in a min r2c0 - PrimaryHueMid border r2c1 - no border r2c2 - no shadow

problems with some of the above, using the card background for the border certainly lifts the contrast on the dark theme but it doesn't at all on the light theme, and worse yet where it works with no shadow on dark, it doesn't work at all in light. So if the border is to be in someway different it would probably need to be a new base theme brush. I don't really like the PrimaryHueMid all the way around the outside, as I think although its more subtle than the current group boxes, it's a bit 'Tron', and it could always be left as an option, depending on what the BorderBrush is being used for, which leads onto my last point which is if the border was able to be different from the line, we would need an attached property for the line as there aren't enough colors to work with on FrameworkElement. With the above taken into consideration r2c1 - which was actually my default, before playing with lifting the contrast slightly - should be the default, and BorderBrush would just affect the line, and not the actual border, which is still questionable, but I can picture lots of these on a screen breaking up areas more than the others. Another possible idea is making the header a different color but in the paper spectrum, so very dark or light, but we should keep some of the theme color in there.

Very keen on thoughts and opinions on this

rrs avatar Jul 19 '21 22:07 rrs

So once again good work. I agree I think I like the r2c1 no border option the best for the default. I think what I would suggest is simply judging the default border thickness and border brush to be used as the actual border (just to enable 'Tron mode'). I would create a new attached property to allow for controlling the line brush color. I think starting with a default of PrimaryHueMid makes sense. As for the header color I think I would probably go with the attached property approach for it as well, though it may not be needed.

Keboo avatar Jul 20 '21 06:07 Keboo

Taken it a little further by replacing the line with a border, it was a toss up between nesting the header border or not within the outer border, I went with nested as I think it gives more options of what you can do. I have added a PaperDark color to the base themes. I'm not 100% on this, I like it in principle but not sure if there is another way to achieve the same thing (different shade in both light/dark for end users).

Also added corner radius to header and main border. There is a slight niggle, if you set the outer corner radius and not the header to match, at least the top half, then the header renders outside the corners, I had a play with using a visual brush to force clip the inner but not sure its really worth adding the bloat.

groupboxes

Example of clipping (or lack off) issue, when you set the corner radius on the group box but not the header to match image

Possible solution. This would go on the outer border

<Border.OpacityMask>
    <VisualBrush>
        <VisualBrush.Visual>
            <Border 
                Background="Black"
                SnapsToDevicePixels="True"
                CornerRadius="{Binding CornerRadius, RelativeSource={RelativeSource AncestorType=Border}}"
                Width="{Binding ActualWidth, RelativeSource={RelativeSource AncestorType=Border}}"
                Height="{Binding ActualHeight, RelativeSource={RelativeSource AncestorType=Border}}"
                />
        </VisualBrush.Visual>
    </VisualBrush>
</Border.OpacityMask>

rrs avatar Jul 20 '21 13:07 rrs

This is outside the scope of this but what would be cool is to open up the theming so that it is more extensible. Instead of light / dark being binary you might have light, dark and blue (thinking of VS). Taking this one stage further if I create my theme, MyCustomeTheme which implements ITheme or IBaseTheme?, then the theming system would be able to handle getting all the extra color properties and applying them. Just glancing over it all, I don't quite understand why ITheme and IBaseTheme exists together, is IBaseTheme the old version and might be removed at some stage (if its in the way)? Anyway if this was easy to do I could just do something like

public interface IMyTheme : ITheme
{
    Color PaperVariant1 { get; }
    Color PaperVariant2 { get; }
}

and then in Xaml throughout the app I could use

{DynamicResource PaperVariant1}

Just to reiterate on the intention: Ability for designers to add custom colors to a theme, that change when the theme changes.

rrs avatar Jul 21 '21 07:07 rrs

A couple more styles

groupboxes

rrs avatar Jul 21 '21 08:07 rrs

Really like all of the new styles. They look great.

To start with your question on theming. Making the theming API robust has been a long running goal, but is not easy to always get perfect. The idea with base themes light/dark is that they would get paired with a color theme (ITheme; probably a bad name in retrospect). But yes, I have been looking at options to try and make this more expandable, especially to try and accommodate the Window's High Contrast themes. Would be happy to take suggestions on better APIs for the 5.x release and I expect there will be some changes coming for that (looking to do some breaking renames of many of the styles/templates/brushes to bring them inline with the latest material design spec).

In terms of specific review pieces.

  1. I think for now lets omit the MaterialDesignPaperDark from the theming since it currently only being consumed in the demo app. Rather I would set it up as using the theme manager and inject it on the ThemeChanged event. This is currently one of the few theming extension points but it is not obvious. There is also an example in the repo here.
PaletteHelper helper = new PaletteHelper();
if (helper.GetThemeManager() is { } themeManager)
{
    themeManager.ThemeChanged += ThemeManager_ThemeChanged;
}
...
private void ThemeManager_ThemeChanged(object? sender, ThemeChangedEventArgs e)
{
    SolidColorBrush paperDark = e.NewTheme.GetBaseTheme() == BaseTheme.Light
        ? new SolidColorBrush(Color.FromArgb(0xFF, 0xEE, 0xEE, 0xEE))
        : new SolidColorBrush(Color.FromArgb(0xFF, 0x24, 0x24, 0x24));
    e.ResourceDictionary["MaterialDesignPaperDark"] = paperDark;
}
  1. I think it would be good to evaluate which of the GroupBoxAssist attached properties can be reasonably applied to the other group box styles, and enable those as well. The HeaderForeground and HeaderBackground I have flip flopped on, since it technically possible to set arbitrary content into the GroupBox's header to manually control these, though it is certainly much more work than a couple property setters. For example this effectively converts the "Header Border" sample into the "Header Corner Radius" sample.
<GroupBox
    Style="{DynamicResource MaterialDesignPaperGroupBox}"
    materialDesign:GroupBoxAssist.HeaderBorderThickness="0"
    Margin="16"
    Padding="0">
    <TextBlock Text="My Content" Padding="9"/>
    <GroupBox.Header>
        <Border BorderThickness="1" BorderBrush="{DynamicResource PrimaryHueMidBrush}" CornerRadius="4"
                Padding="9">
            <TextBlock Text="Header Border" />
        </Border>
    </GroupBox.Header>
</GroupBox>
  1. In terms of the "niggle" (thanks for teaching me a new word). We could consider something a bit more complex. I am not 100% sure this would be better but I want to get your thoughts on it. What is we separate the concepts of the border around the group box from the header, and instead just end with stuff related to the group box's border, and stuff related to the "header separator". This would eliminate the confusion with the top corner radius since it would then only exist for the group box itself. I was thinking of changing up the types of the exposed properties for the corner radius and thickness since these appear to be the ones that overlap and look odd when set to different values (potentially also the brush colors if opacity is used as well). For CornerRadius we could declare our own type, similar to corner radius where it accepts either a single universal value, 4 values for the corner, or 6 values for the corners plus the header bottom corners. This would allow a lot of freedom in specifying them, but it would mean writing a custom type, along with a TypeCovnerter to parse the XAML string into the new custom type. If you are unfamiliar with this, you can see how it works by looking at the existing corner radius code and its type converter. Basically on the custom type, you declare a TypeConverterAttribute specifying the converter you want it to use to parse the strings from the XAML. From there you can implement whatever logic you want in the custom TypeConverter class. Similar could be done with the header separate thickness as well. With that all said, it may be a lot of complexity for only a small gain. Thoughts?

I did review it on my stream if you want the long explanation, with me getting distracted a lot.

Keboo avatar Jul 23 '21 05:07 Keboo

Hey, I watched the stream, it was good to get your full thought process on it all. I will have a proper dive into some of these in code but thought id put some of my thoughts out there straight away. I'm with you across the board.

  1. Its good to see there is a way to do this, and I went to plug it in straight away, added that code to App.xaml.cs in the demo project and it works fine once the theme switches, but the resource is not there to start because the event does not fire. So I have just added it in App.xaml hard coded to the dark default, which is fine, except for if a user changes it to light in the xaml, the value will start on the dark default. Maybe you can show me if this is possible to work around.

image image

Anyway not too worried about this at this stage, happy to ditch it completely for this PR if it can't play nice and pick it up in one that addresses theming.

  1. (+3) I hear you. Your thoughts actually echo my own. I don't know if you noticed but you can practically create the original group box style using this style if you set the background to one of the main colors, and set the main border, that's a warning sign to me if nothing else.

I am sadly all too familiar with typeconverters in WPF, specifically because of my work on theming before, something they lack which is the ability to auto suggest for properties of custom types, even if you implement all the right methods. In one of the early cuts of the theming overhaul I tried to use them for the colors but you can only use enums if you want intellisense, which makes me very sad! Oh how sweet we could make the BundledTheme if they would give us this power! Anyway as above I'm happy to bench my thoughts on this for another PR, I have a few ideas already

So back to the matter at hand, I don't like the idea of a 6 value type converter personally, I think it would just be too unnatural, how would you order it? TL, TR, BR, BL, ML, MR? or maybe TL, TR, ML, MR, BR, BL? (out of the two, the first makes more sense to me) and given your valid points on the header template being its own thing, and the user can do as they like with it I think we can come at it from another angle. I am going to look at separating the header template, I think the original style and this style can probably be brought together, where the header template is a separate key and can be switched out in each style, and even omitted completely if the user wants to do their own thing but keep that card style background + border. We might be able to take this a stage further, where the color zone itself actually takes on some of these more minimal properties. It was something I was going to look at anyway, as it is one of the 'in your face color attack' controls. That way it would probably just roll into the original style, without the need for a new separate style. However I don't know if it stretches the intent of a 'ColorZone' too much, if most of it is paper with only a primary line or border? If we went down this route I think we could move those Header### attached properties to ColorZoneAssist, and use them from there. RE. corner radius, I am going to have to have a play with this. I can't think of a nice way to do it yet, an alternative to 6 values would be to separate the bottom border of the header from the main border, so it would use a converter to get the top two values from the groupbox and another optional attached property for just the bottom two, either solutions are weird, but i think its rare that bottom header is going to be rounded anyway, which brings us back to, 'maybe its outside the scope of the library' to try it at all, why worry about it, just give a nice default header template or two and the user can roll their own if they want it.

p.s I think 'niggle' is probably a mostly British or English word. Also great tip on adding syntax highlighting to code blocks, thanks for that one, I'll be using that further down this PR no doubt

rrs avatar Jul 24 '21 00:07 rrs

Hello,

Do you know if this will make it into a future release? I am currently using cards with rounded corners and a rounded corner group box would be very nice to have as part of the style.

emanuelhristea avatar Mar 26 '22 23:03 emanuelhristea

This PR is marked stale because it has been open 60 days with no activity. Remove stale label or update the PR, otherwise it will be closed in 14 days.

github-actions[bot] avatar Apr 23 '23 02:04 github-actions[bot]

This PR is marked stale because it has been open 60 days with no activity. Remove stale label or update the PR, otherwise it will be closed in 14 days.

github-actions[bot] avatar Jun 24 '23 02:06 github-actions[bot]

This PR is marked stale because it has been open 60 days with no activity. Remove stale label or update the PR, otherwise it will be closed in 14 days.

github-actions[bot] avatar Aug 24 '23 01:08 github-actions[bot]

This PR was closed because it has been stalled for 14 days with no activity.

github-actions[bot] avatar Sep 07 '23 01:09 github-actions[bot]

This PR is marked stale because it has been open 60 days with no activity. Remove stale label or update the PR, otherwise it will be closed in 14 days.

github-actions[bot] avatar Nov 08 '23 01:11 github-actions[bot]

This PR is marked stale because it has been open 60 days with no activity. Remove stale label or update the PR, otherwise it will be closed in 14 days.

github-actions[bot] avatar Jan 08 '24 01:01 github-actions[bot]