-
Notifications
You must be signed in to change notification settings - Fork 653
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
Add info font icons #1297
Conversation
There was a problem hiding this 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!
change sample txt Co-Authored-By: Niels Laute <[email protected]>
69e0ac5
to
3d897fc
Compare
PTAL |
There was a problem hiding this 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)?
I remembered a comment about the sizes and thought I would add a little bit of that too... If this all feels as too much text for this page, then we can think about splitting things (via a Pivot or the |
There was a problem hiding this 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.
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. |
Yes, but these are generic principles that can be followed for everything within an app - from layouts, to controls. So not specific to icons.
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)
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. 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
There was a problem hiding this 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!
When will this be visible in the app? |
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):
Types of changes