Skip to content
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

Updated TabbedCommandBar for WinUI 2.6 #4132

Merged
11 commits merged into from
Jul 30, 2021

Conversation

yoshiask
Copy link
Contributor

Fixes #4085

Previously, TabbedCommandBar used the system XAML NavigationView, but this doesn't work on WinUI 3. This PR makes the control functional on WinUI 2.6+ and 3.x, albeit with a few style issues (which will be fixed either in this PR or a later one).

PR Type

What kind of change does this PR introduce?

Bugfix
Refactoring (no functional changes, no api changes)

What is the current behavior?

See #4085

What is the new behavior?

  • TabbedCommandBar now inherits from MUXC NavigationView and uses 2.6 styles
  • Normal* and ContextualTabTemplate now use MUXC NavigationViewItem

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Contains NO breaking changes

@ghost
Copy link

ghost commented Jul 26, 2021

Thanks yoshiask for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Rosuavio July 26, 2021 21:53
@ghost ghost added the bug 🐛 An unexpected issue that highlights incorrect behavior label Jul 26, 2021
@michael-hawker
Copy link
Member

Thanks @yoshiask! Will see if we can tweak the style bit a bit and slip this in the next preview. We can polish things up more before release.

@michael-hawker michael-hawker added controls 🎛️ next preview ✈️ Label for marking what we want to include in the next preview release for developers to try. labels Jul 26, 2021
@michael-hawker michael-hawker marked this pull request as draft July 26, 2021 21:57
@michael-hawker michael-hawker added this to the 7.1 milestone Jul 26, 2021
@michael-hawker michael-hawker self-assigned this Jul 27, 2021
…uggestBox in TabbedCommandBar

Note: Back Button doesn't actually show correctly still due to issue with Visual States that is unknown
We should investigate further better updates to the template in a redesign for the new design guidelines.
@michael-hawker
Copy link
Member

michael-hawker commented Jul 27, 2021

Fixed a couple of things, not perfect, but at least getting better. Still an issue with the inner button padding/sizes:

image

I just adjusted the height of the TabbedCommandBarItem for now from 40 to 48 to at least fix the clip.

Still issues with underlying Back Button behavior due to VSM not firing/setting BackButtonGroup state, no idea why that was happening as template parts for that were all the same. Flipped how that worked, so at least by default it doesn't have the gap, as we wouldn't normally expect back button to be used? Not sure why it works differently than Settings button which I turned off by default, but works fine when enabled.

Add Theme Dictionary to better control contextual tab color, works for theme awareness now, but probably still needs better color values.

Also added support quick for AutoSuggestBox 🙂

We can probably at least merge this in if we'd like for now, and then make another couple of fixes later or update more for the 2.6 style anyway.

@michael-hawker
Copy link
Member

Realized VSM probably wasn't working properly as it's no longer in the root of the template as we have a Grid around the other grid. Tried to move it, but encountered an unknown crash in WinUI, filed an issue here: microsoft/microsoft-ui-xaml#5575

Also filed microsoft/microsoft-ui-xaml#5576 as wasted a lot of time debugging that problem yesterday and only thought about it's position this morning looking at it again.

@michael-hawker
Copy link
Member

michael-hawker commented Jul 28, 2021

Hey! I updated us to the 2.6 style for CommandBar and it actually looks really slick!

image
(mouse was hovering over Paste)

Think this is all good to go for now, the Contextual Tab style could use a bit of work, but that's overall pretty minor.

FYI @yoshiask, I've flipped this out of draft mode. @RosarioPulella @XAML-Knight this is good to review now! 🎉🎉🎉

@michael-hawker michael-hawker marked this pull request as ready for review July 28, 2021 20:57
@@ -36,8 +37,10 @@ public class TabbedCommandBar : NavigationView
public TabbedCommandBar()
{
DefaultStyleKey = typeof(TabbedCommandBar);
DefaultStyleResourceUri = new System.Uri("ms-appx:///Microsoft.Toolkit.Uwp.UI.Controls.Core/Themes/Generic.xaml");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tried to remove the need for this line of code, but was unsuccessful

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we had this same issue with ColorPicker as well. We have an issue on WinUI for this here: microsoft/microsoft-ui-xaml#3502 - I suppose we could link to it, but I think we need it on all controls for WinUI 3, right @azchohfi?

@michael-hawker
Copy link
Member

Talked to @yoshiask, they're good with changes. So think we're good.

@ghost
Copy link

ghost commented Jul 30, 2021

Hello @michael-hawker!

Because this pull request has the auto merge :zap: label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit e4e06cb into CommunityToolkit:main Jul 30, 2021
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merge ⚡ bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ next preview ✈️ Label for marking what we want to include in the next preview release for developers to try.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TabbedCommandBar not working Windows App SDK 0.8
3 participants