Add AlwaysExpanded property to CommandBarFlyout #4532
Conversation
ghost
added
the
ranjeshj
added
area-Commanding
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // When CommandBarFlyout is in AlwaysOpen state, don't show the overflow button | ||
| if (AlwaysExpanded()) | ||
| { | ||
| commandBar.IsOpen(true); |
There was a problem hiding this comment.
commandBar.IsOpen(true); [](start = 20, length = 24)
Why does AlwaysExpanded affect if we open the commandbar (not the overflow flyout)
There was a problem hiding this comment.
The CommandBar IsOpen is actually about the overflow flyout! It's unfortunately a bit confusing.
| if (AlwaysExpanded()) | ||
| { | ||
| commandBar.IsOpen(true); | ||
| commandBar.OverflowButtonVisibility(winrt::Windows::UI::Xaml::Controls::CommandBarOverflowButtonVisibility::Collapsed); |
There was a problem hiding this comment.
Don't we have to do something to show the overflow items?
| @@ -813,7 +814,7 @@ | |||
| XYFocusKeyboardNavigation="Enabled"> | |||
| <Grid.ColumnDefinitions> | |||
| <ColumnDefinition Width="*" /> | |||
| <ColumnDefinition Width="Auto" /> | |||
| <ColumnDefinition Width="Auto" MinWidth="{StaticResource AppBarRightMargin}"/> | |||
There was a problem hiding this comment.
AppBarRightMargin [](start = 89, length = 17)
How is this a margin?
There was a problem hiding this comment.
It's a right margin -- basically, if the MoreButton isn't visible, it ensures a right margin between the command buttons and the edge of the control. But I can rename it if that's the issue.
There was a problem hiding this comment.
I think a rename is appropriate, in xaml margin's are of type thickness, which this is not.
There was a problem hiding this comment.
OK, renamed to AppBarMoreButtonColumnMinWidth.
|
Why isn't this named |
Since AlwaysExpanded doesn't sound like an event, it doesn't require the "is". It's been approved by our naming gurus. |
The guidelines state "✔️ DO name Boolean properties with an affirmative phrase (CanSeek instead of CantSeek). Optionally, you can also prefix Boolean properties with "Is", "Can", or "Has", but only where it adds value." While the guidelines give lots of leeway there is an expectation that "Is" goes before bool properties. The "Is" prefix has been well established for well over a decade now. There has never been any requirement that "Is" is only added when it could be confused with an event (although it would certainly add value there). The concerns usually occur if there are grammatical issues and then "Has" or some alternatives are considered. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
I think I agree, but the API is marked as preview, so we can change it during API review. |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
If set to True, AlwaysExpanded makes the CommandBarFlyout open with the secondary command list visible, and removes the ... button which normally toggles the secondary menu.
This also fixes a bug in CommandBar and CommandBarFlyout where if the ... button is hidden there's no margin between the last command and the edge of the control.