Imperfect looks on the Color controls

[Jay-o-Way]

Which version of the app?

WinUI 3 Gallery

Description

Minor issue.

• In some occasions, it looks like the border of the ColorTile control and the child element(s) do not have the same CornerRadius, or a padding (?) that creates a border.
• In some occasions, it looks like 4px is used.
• In some (2) places, CornerRadius="4" is used. Not only is it hard-coded, it's also the incorrect value for these cases.

Screenshots

Note

A Grid with a CornerRadius (Style GalleryTileGridStyle uses OverlayCornerRadius) does not clip the content. A Border does. That means we can envelope each Grid with a Border. the accompanying Style should then be set on the Border, in place of the Grid.

Issue is: GalleryTileGridStyle is used throughout the DesignGuidance and Accessibility pages.

[niels9001]

@Jay-o-Way Something you'd like to pick up?

[Jay-o-Way]

@niels9001 I think these lines are unused and can be removed, which means the two top Grids can be removed. Can you confirm?

WinUI-Gallery/WinUIGallery/Controls/DesignGuidance/ColorPageExample.xaml

Line 15 in 6be8d2d

<RowDefinition Height="Auto" />

WinUI-Gallery/WinUIGallery/Controls/DesignGuidance/ColorTile.xaml

Line 17 in 6be8d2d

<ColumnDefinition Width="Auto" />

[niels9001]

@niels9001 I think these lines are unused and can be removed, which means the two top Grids can be removed. Can you confirm?

WinUI-Gallery/WinUIGallery/Controls/DesignGuidance/ColorPageExample.xaml

Line 15 in 6be8d2d

<RowDefinition Height="Auto" />

Yes, even the root grid can be removed here?

WinUI-Gallery/WinUIGallery/Controls/DesignGuidance/ColorTile.xaml

Line 17 in 6be8d2d

<ColumnDefinition Width="Auto" />

I think we are using that columndefinition for the border?

[Jay-o-Way]

I think we are using that columndefinition for the border?

Ah yes, the ShowSeparator. Thing is - in practice it isn't used, so I already removed it (locally) when working on this. Therefore the ColumnDefinition is obsolete.

[niels9001]

I think we are using that columndefinition for the border?

Ah yes, the ShowSeparator. Thing is - in practice it isn't used, so I already removed it (locally) when working on this. Therefore the ColumnDefinition is obsolete.

Just looking at the code.. it's turned on by default:

And in 8 instances we turn it off https://github.com/search?q=repo%3Amicrosoft%2FWinUI-Gallery%20ShowSeparator&type=code?

So I think we are using it?

[niels9001]

@Jay-o-Way It is used in some occasions, and it is visible for me?

Set to true:

Set to false:

I think, since we are use it here and there to make the default be false and only turn it on when needed to reduce the verbosity of XAML. But other than that, I think we need to keep it..

[Jay-o-Way]

I see. Focussed on the declared ("false") values and missed the instances where it wasn't declared. 🫣
We could elevate this topic to a higher level: why not choose to use it either always or never?

---

Ugh, my brain keeps thinking more and more things. Maybe, one day, we can combine all the issues with these Color things, and refactor the whole thing..? Using a very large UserControl with bound data is a big work, but might solve a couple of issues.

[niels9001]

I see. Focussed on the declared ("false") values and missed the instances where it wasn't declared. 🫣 We could elevate this topic to a higher level: why not choose to use it either always or never?

This was per design spec. We only need it when 2 neighboring colors are similar to provide the right contrast. I don't think there's a problem here that needs to be solves? Having it default to false would be my only proposed change.

On the CornerRadius issues - yes, we should remove any hardcoded 4px radiuses if those are defined. Not sure why those were defined in the first place though 😨. (Could be that the grid style was added later, and we missed removing a few of these).

[Jay-o-Way]

I've been digging into this now, but the corners are weird. Looks like it's not a matter of a property, but more of the rendering...