Add resource for topmode CustomContentPane minimum width #2905
Conversation
msft-github-bot
added
the
needs-triage
| @@ -231,7 +231,8 @@ | |||
| <x:Double x:Key="PaneToggleButtonWidth">40</x:Double> | |||
| <x:Double x:Key="NavigationViewCompactPaneLength">40</x:Double> | |||
| <x:Double x:Key="NavigationViewTopPaneHeight">40</x:Double> | |||
|
|
|||
| <x:Double x:Key="NavigationViewTopModeContentPaneMinWidth">48</x:Double> | |||
There was a problem hiding this comment.
We should strive to be consistent as best as possible with our theme resource naming here. As far as I can see, only the theme resource NavigationViewTopPaneHeight uses a slightly different prefix for the specific NavigationView theme resources in top pane display mode. The overwhelming majority uses the prefix "TopNavigationView" so I suggest we should use that prefix here as well. "NavigationViewTopMode" is yet another new prefix here and we should avoid that.
(Out of scope here but mentioned for completeness: For consistency, we should also add a TopNavigationViewPaneHeight theme resource later when, for example, the XAML resource lookup system supports a level of indirection to avoid a breaking change.)
The naming "NavigationViewTopModeContentPaneMinWidth" also seems to suggest to me that the NavigationView has a content pane with its min width configurable here. As that is not the case (there is only one pane), we should be careful how we name it here. The ContentControl placed inside this grid column is named "PaneCustomContentOnTopPane" so perhaps we could name it TopNavigationViewPaneCustomContentMinWidth?
There was a problem hiding this comment.
Very good point. I like "TopNavigationViewPaneCustomContentMinWidth", so I've updated the code now to use that name. Thanks for the suggestion!
(Out of scope here but mentioned for completeness: For consistency, we should also add a TopNavigationViewPaneHeight theme resource later when, for example, the XAML resource lookup system supports a level of indirection to avoid a breaking change.)
Should there be/is there a tracking issue for that?
There was a problem hiding this comment.
I do plan to create a tracking issue for the XAML resource lookup system soon (I'm aware I'm already a few days late here 😅) but there is none for ensuring constistency in the naming scheme of the existing NavigationView theme resources (like NavigationViewTopPaneHeight vs TopNavigationViewPaneHeight). We should create one (as otherwise this can fall off the radar pretty quickly). Feel free to create a tracking issue for this if you agree, @chingucoding 🙂
There was a problem hiding this comment.
Created #2906 for the navview resource naming scheme.
There was a problem hiding this comment.
Agree that we should try to be as consistent as possible, but it looks like we already have some inconsistencies in the naming and we have to be careful about changing the naming since that is potentially a breaking change. Lets try to follow the pattern as much as possible. @YuliKl and @anawishnoff as FYI.
StephenLPeters
added
area-NavigationView
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -113,7 +113,7 @@ | |||
| <VisualState x:Name="OverflowButtonWithLabel" /> | |||
| <VisualState x:Name="OverflowButtonNoLabel"> | |||
| <VisualState.Setters> | |||
| <Setter Target="TopNavOverflowButton.Style" Value="{ThemeResource NavigationViewOverflowButtonNoLabelStyleWhenPaneOnTop}" /> | |||
| <Setter Target="TopNavOverflowButton.Style" Value="{ThemeResource TopNavigationViewPaneCustomContentMinWidth}" /> | |||
There was a problem hiding this comment.
TopNavigationViewPaneCustomContentMinWidth [](start = 106, length = 42)
Am i missing something here or is this change setting a style to a double?
There was a problem hiding this comment.
Not sure how that slipped in, should be fixed now.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Description
Adds themeresource for ContentPane minimum width in top displaymode
Motivation and Context
Closes #2890
How Has This Been Tested?
Tested manually
Screenshots (if appropriate):