-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Various changes for cards and shadows #2765
Various changes for cards and shadows #2765
Conversation
I'm not 100% sure about the depth1 shadow. Tell me what do you think. |
Adding link to spec document: https://material.io/design/environment/elevation.html |
<DropShadowEffect x:Key="MaterialDesignShadowDepth6" BlurRadius="12" ShadowDepth="3.5" Direction="270" Color="{StaticResource MaterialDesignShadow}" Opacity=".42" RenderingBias="Performance" po:Freeze="True" /> | ||
<DropShadowEffect x:Key="MaterialDesignShadowDepth7" BlurRadius="14" ShadowDepth="4" Direction="270" Color="{StaticResource MaterialDesignShadow}" Opacity=".42" RenderingBias="Performance" po:Freeze="True" /> | ||
<DropShadowEffect x:Key="MaterialDesignShadowDepth8" BlurRadius="16" ShadowDepth="4.5" Direction="270" Color="{StaticResource MaterialDesignShadow}" Opacity=".42" RenderingBias="Performance" po:Freeze="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.
I am wondering if these should actually be larger. Looking at the default depths on the spec it looks like the elevation differences at the higher levels slowing increase the dp (radius and depth here) as you get to the higher elevations.
Thoughts?
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.
Thank you for your feedback @Keboo. Yes I agree with you. I had looked at the shadows only for the cards and it says it goes from 1dp to 8dp. That's why I had made 8 levels. But actually, there are more than 8 levels.
Finally, I think I'm going to keep the Depth0 to Depth5 levels and add some new levels. I think I'll name them DP1, DP2, DP3 etc. I think it's more meaningful.
Finally, I decided to create new shadow levels and keep the old ones. The old ones can still be used but I think they should be considered as obselete. The obselete attribute makes the pipeline fail so I removed it from the code. I just put obselete regions for now. Here is the final result: If we want to stay consistent with the new shadow system we will have to change the ShadowDepth on each components of the library.
So if we decide to adopt this new system, we will have to change for example the property |
#region ObseleteDepth | ||
Depth0, | ||
Depth1, | ||
Depth2, | ||
Depth3, | ||
Depth4, | ||
Depth5 | ||
Depth5, | ||
#endregion |
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 think actually putting the Obsolete attribute on these is a good idea. What are your thoughts on these suggested update values (since there are no exact equivalents)?
[Obsolete("Use Depth_0dp instead")]
Depth0,
[Obsolete("Considersing Depth_2dp instead")]
Depth1,
[Obsolete("Considersing Depth_3dp instead")]
Depth2,
[Obsolete("Considersing Depth_7dp instead")]
Depth3,
[Obsolete("Considersing Depth_12dp instead")]
Depth4,
[Obsolete("Considersing Depth_24dp instead")]
Depth5,
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.
That sounds good to me
#region ObseleteDepth | ||
{ ShadowDepth.Depth0, null }, | ||
{ ShadowDepth.Depth1, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth1"] }, | ||
{ ShadowDepth.Depth2, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth2"] }, | ||
{ ShadowDepth.Depth3, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth3"] }, | ||
{ ShadowDepth.Depth4, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth4"] }, | ||
{ ShadowDepth.Depth5, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth5"] }, | ||
#endregion |
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.
To fix the pipeline failing on warnings. We can simply suppress these. This will ensure that people properly update their enum values before removing them in 5.x.
#region ObseleteDepth | |
{ ShadowDepth.Depth0, null }, | |
{ ShadowDepth.Depth1, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth1"] }, | |
{ ShadowDepth.Depth2, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth2"] }, | |
{ ShadowDepth.Depth3, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth3"] }, | |
{ ShadowDepth.Depth4, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth4"] }, | |
{ ShadowDepth.Depth5, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth5"] }, | |
#endregion | |
#region ObseleteDepth | |
#pragma warning disable CS0618 // Type or member is obsolete | |
{ ShadowDepth.Depth0, null }, | |
{ ShadowDepth.Depth1, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth1"] }, | |
{ ShadowDepth.Depth2, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth2"] }, | |
{ ShadowDepth.Depth3, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth3"] }, | |
{ ShadowDepth.Depth4, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth4"] }, | |
{ ShadowDepth.Depth5, (DropShadowEffect)resourceDictionary["MaterialDesignShadowDepth5"] }, | |
#pragma warning restore CS0618 // Type or member is obsolete |
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.
Perfect, thank you, I'll do that
@Xaalek @Keboo @henon @sebastiandymel |
@Norc89 at present there is not a way to change the color. However, this library is just using the WPF |
Changes
Consequences:
I tried to match the specs as much as possible.
Please tell me if you like the changes.
I can adjust the shadow opacity if necessary.
Specs
Results
Examples with the same Background as the specs: