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

Registry Preview UI #27266

Merged
merged 8 commits into from
Jul 19, 2023
Merged

Registry Preview UI #27266

merged 8 commits into from
Jul 19, 2023

Conversation

Jay-o-Way
Copy link
Collaborator

@Jay-o-Way Jay-o-Way commented Jul 7, 2023

Summary of the Pull Request

image

PR Checklist

Validation Steps Performed

Visual Studio

@Jay-o-Way Jay-o-Way requested a review from randyrants July 7, 2023 13:43
@Jay-o-Way Jay-o-Way changed the title Registry Preview Registry Preview UI Jul 7, 2023
@Jay-o-Way Jay-o-Way marked this pull request as draft July 10, 2023 19:02
@Jay-o-Way Jay-o-Way marked this pull request as ready for review July 10, 2023 23:34
@Jay-o-Way Jay-o-Way requested a review from niels9001 July 10, 2023 23:35
Copy link
Contributor

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

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

Thanks!!

Few things, should be easy fixes.

<RowDefinition Height="*" />
</Grid.RowDefinitions>

<Grid
<CommandBar
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put back the background color of the CommandBar.. this was per design specification.

Copy link
Collaborator Author

@Jay-o-Way Jay-o-Way Jul 13, 2023

Choose a reason for hiding this comment

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

Can you elaborate or show the spec? Usually it's like a menubar. WinUI 3 Gallery and https://learn.microsoft.com/windows/apps/design/controls/command-bar (and pretty much every app that comes to mind) always show a clear commandbar.
(and honestly/personally I don't like the looks of it)

Copy link
Contributor

Choose a reason for hiding this comment

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

We have design syncs with the design team where we discuss these things. They showed this as an example (Outlook):

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh I see... The Outlook family - and Office family - do have a different UI then most (MS Store) apps. But if you want me to add it back I will. You're saying this style is going to be applied to "all" other MS apps in the future?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please do. W11 introduced the notion of "cards" - and something that is part of the broader Fluent Design story.

For best practices, and the latest guidance, the Windows Visual Library in Figma is your best source: https://www.figma.com/community/file/1159947337437047524/Windows-UI-3

As a rule of thumb, the content there is the latest defined by the design team owning the W11 design language. Some of the content you will not find in, or might contradict, the documentation on MS Docs. Those articles sometimes refer to the UWP/W10 days, and are mostly targeting < WinUI 2.5 (before the design refresh)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Detail: a commandbar is by default aligned to the right. Setting HorizontalAlignment="Left" will break the "stretch" property, and then an extra border is visible when the overflow menu is open. Still want to keep it left? Right?
image

@Jay-o-Way
Copy link
Collaborator Author

Jay-o-Way commented Jul 13, 2023

Ready.
@randyrants wadda ya think? :)

@randyrants
Copy link
Collaborator

I think I've been far too much time dealing with probate docs than compilers the last couple of weeks (and see no end of it in sight) - I defer to your paired expertise in the UX. Had a couple of comments on the resw file but nothing urgent or binding.

@Jay-o-Way Jay-o-Way requested a review from niels9001 July 18, 2023 12:24
Copy link
Contributor

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

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

Looking good.. one last thing:

Setting the RowSpacing to 12 causes this:

image

Note for later: once Toolkit 8.0 is out, we probably want to adopt the new GridSplitter that adopts a W11 design - https://github.com/CommunityToolkit/Windows/blob/main/components/Sizers/samples/GridSplitter.md

@Jay-o-Way
Copy link
Collaborator Author

Did it! 😄

Copy link
Contributor

@niels9001 niels9001 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks!

@niels9001 niels9001 merged commit 4647491 into microsoft:main Jul 19, 2023
@Jay-o-Way Jay-o-Way deleted the tweak-ui-regprev branch July 19, 2023 06:19
@randyrants
Copy link
Collaborator

randyrants commented Aug 2, 2023

@Jay-o-Way late question, but my time has been shredded across too many things.

There are three buttons in the command bar in 0.72 that no longer have icons: that by design and if so, I'm wondering why, since all of the selected icons came under heavy review and feedback, seemingly landing in a place that made sense as part of the command bar work.

Not opposed to keeping them out if there's a reason and happy to put them back if it was accidental but this touches on a bigger problem: one of the reasons why I went after the additional icons for the buttons stemmed from a bug reported on using the Icon property in CommandBar: #25284

This led me to some docs that called out that we should be using AppButton.Icon and a FontIcon as that is deterministic in all languages while also allowing a larger collection of glyphs to leverage.

Sorry I missed it when it was active but just noticed it now.

@Jay-o-Way
Copy link
Collaborator Author

@randyrants no problem, fair question. Well, the reason for me was, that the icons used were just not specific enough. So for me it was like "if we can't find an icon good enough, might as wel omit it". But yes, that's a personal opinion.

The same applies to some places in the Settings pages. Maybe, one day, we can create icons ourself and add those?

I do agree that a command bar is good with icons, and that icons are good for any language. At least in general.

P.S. the linked issue was about a specific range of icon codes plus a specific UI situation (HongKong font).

@randyrants
Copy link
Collaborator

randyrants commented Aug 2, 2023

I agree, the icons are preference, but I don't think the Font issue is. It was reported report for Traditional Chinese but I believe I saw the same behavior in Japanese, then made the change to the code following the guidance of the MSFT documentation for best practice.

I'll file a bug and prepare a fix for it. Coin flip if I put the old not-good-enough icons or not. 🙂

PR: #27770

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants