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

Add info font icons #1297

Merged
merged 7 commits into from
Jun 7, 2023
Merged

Conversation

Jay-o-Way
Copy link
Contributor

@Jay-o-Way Jay-o-Way commented May 23, 2023

Added info about icons...

Description

And placed this in an Expander.

Motivation and Context

Closes: #1263

How Has This Been Tested?

Visual Studio

Screenshots (if appropriate):

image

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Copy link
Collaborator

@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.

Nice additions, thanks!

Few minor nits and then this should be ready to go!

WinUIGallery/ControlPages/DesignGuidance/IconsPage.xaml Outdated Show resolved Hide resolved
WinUIGallery/ControlPages/DesignGuidance/IconsPage.xaml Outdated Show resolved Hide resolved
WinUIGallery/ControlPages/DesignGuidance/IconsPage.xaml Outdated Show resolved Hide resolved
WinUIGallery/ControlPages/DesignGuidance/IconsPage.xaml Outdated Show resolved Hide resolved
WinUIGallery/ControlPages/DesignGuidance/IconsPage.xaml Outdated Show resolved Hide resolved
WinUIGallery/ControlPages/DesignGuidance/IconsPage.xaml Outdated Show resolved Hide resolved
change sample txt

Co-Authored-By: Niels Laute <niels.laute@live.nl>
@Jay-o-Way
Copy link
Contributor Author

PTAL

@Jay-o-Way Jay-o-Way marked this pull request as draft May 27, 2023 22:07
Copy link
Collaborator

@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.

LGTM! Thanks for the contribution!

@Jay-o-Way Is this ready to be merged (since it's marked as draft)?

@Jay-o-Way
Copy link
Contributor Author

Jay-o-Way commented May 29, 2023

I remembered a comment about the sizes and thought I would add a little bit of that too...
And then I remembered something about layering...

If this all feels as too much text for this page, then we can think about splitting things (via a Pivot or the <labs:Segmented) like the Colors page does.

@Jay-o-Way Jay-o-Way marked this pull request as ready for review May 29, 2023 12:49
@niels9001 niels9001 self-requested a review May 30, 2023 07:55
Copy link
Collaborator

@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.

@

I remembered a comment about the sizes and thought I would add a little bit of that too... And then I remembered something about layering...

If this all feels as too much text for this page, then we can think about splitting things (via a Pivot or the <labs:Segmented) like the Colors page does.

@Jay-o-Way Your latest commit has introduced a lot of information that is not specific to icons - and I think we should avoid information that is currently on the docs already (or move this to a generic layout section in the 'Design Guidance' section)?

I think the instructions in the WinUI Gallery should be concise, actionable and should focus on getting certain things done; it provides snippets / how-to's vs. detailed documentation. For detailed information and background material, devs can learn more by clicking the 'Documentation' dropdown. You could add a Hyperlink instead of the appended text if you'd want to call that out?

I do agree that listing the icon ramp (16, 24, 32, 48, 64) is good to call out, as it's not explicitly mentioned in the Segoe Fluent Icons doc but here.

@Jay-o-Way
Copy link
Contributor Author

Jay-o-Way commented May 30, 2023

that is not specific to icons

I hate to say it, but I don't agree. All this info is taken from either the font page, or "Iconography in Windows 11", or "Alignment, margin, padding". The page you link to, however, is specifically about APP icons, aka logos. That is a very different environment ("outside" of an app) and has nothing to do with font icons ("inside").

I agree it is quite a bit of text - haven't even included the note about deprecated icons yet - but we both know how easy it can be to say or do something that is not "by the book". And that investigating docs is more effort than reading it together in an app.

@niels9001
Copy link
Collaborator

niels9001 commented May 30, 2023

I hate to say it, but I don't agree. All this info is taken from either the font page, or "Iconography in Windows 11", or "Alignment, margin, padding".

Yes, but these are generic principles that can be followed for everything within an app - from layouts, to controls. So not specific to icons.

The page you link to, however, is specifically about APP icons, aka logos. That is a very different environment ("outside" of an app) and has nothing to do with font icons ("inside").

That's correct - however, the sizes listed under 'Context menu, title bar, system tray' are the sizes that are recommended for system icons as well. (The Design team pointed us to this specific page specifically and told us to follow that ramp). They are aware that the docs are a bit unclear on this, and are planning to update this in the future. (EDIT: opened a PR with a note about the type ramp: MicrosoftDocs/windows-dev-docs#4464)
image
I'm not saying we should link to that page - but adding a 1 liner mentioning that ramp might be helpful (and we should add it to the Segoe Fluent Icons doc page as well.

I agree it is quite a bit of text - haven't even included the note about deprecated icons yet - but we both know how easy it can be to say or do something that is not "by the book". And that investigating docs is more effort than reading it together in an app.

The goal of this page is to get to the icon picker as fast as possible - background information can be found on Docs. We don't want to bloat this tool with additional information that already lives in Docs - there will always be some overlap of course, but we should keep it to a minimum as possible. Also because information might get updated and the docs are the source of truth.
The goal in general of the Gallery is to show interactive controls with snippets that can quickly be copied - and having a shortcut to docs to learn more.


Having these detailed instructions was not something discussed prior in the issue - we should make sure we discuss these things there before opening a PR to avoid any threads here.

For now, I'd recommend reverting your last commit so we can merge this in?

and update link
Copy link
Collaborator

@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 great! Thanks @Jay-o-Way!

@niels9001 niels9001 merged commit 831f3c3 into microsoft:main Jun 7, 2023
1 check passed
@Jay-o-Way Jay-o-Way deleted the add-info-font-icons branch June 7, 2023 13:48
@Jay-o-Way
Copy link
Contributor Author

When will this be visible in the app?

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.

Include a bit more text about icon font(s) in the main page
2 participants