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

tweak fontsizes #1240

Merged
merged 4 commits into from
May 4, 2023
Merged

tweak fontsizes #1240

merged 4 commits into from
May 4, 2023

Conversation

Jay-o-Way
Copy link
Contributor

@Jay-o-Way Jay-o-Way commented Mar 30, 2023

Description

Just small changes

Motivation and Context

  • Do not to use font-size 14 on icons, they are designed with 16px in mind.
    • only change properties when needed, but at the very minimum use the "rule-of-4"
  • Quote from the app: "The minimum values should be 12px Regular, 14px Semibold"

How Has This Been Tested?

Screenshots (if appropriate):

WinUI3Gallery icons

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)

Best not to use font-size 14 on icons, they're designed with 16px in mind. Quote from your own app: "The minimum values should be 12px Regular, 14px Semibold"
@karkarl
Copy link
Collaborator

karkarl commented Apr 3, 2023

Hey Jay-o-Way! Thanks for your contribution. Do you mind adding more detailed before and after changes photos in your PR description?

@Jay-o-Way
Copy link
Contributor Author

@karenbtlai done. Visually, it's absolutely minute. More a matter of principle and design/code habits.

WinUIGallery/ControlPages/ListViewPage.xaml Outdated Show resolved Hide resolved
WinUIGallery/ControlPages/ListViewPage.xaml Outdated Show resolved Hide resolved
WinUIGallery/Controls/PageHeader.xaml Outdated Show resolved Hide resolved
WinUIGallery/Controls/PageHeader.xaml Outdated Show resolved Hide resolved
WinUIGallery/Controls/PageHeader.xaml Outdated Show resolved Hide resolved
@niels9001
Copy link
Collaborator

niels9001 commented Apr 4, 2023

@Jay-o-Way @karenbtlai

Synced with the design team, and by default we should adhere to using 16/24/32/48 as @Jay-o-Way pointed out.

However, FontIcon/SymbolIcon uses 20 by default (I always was under the impression it would default to 16, like normal text..). So we still have to manually set these to "16".

Chart for comparison (as @Jay-o-Way pointed out, 14 causes blurriness on higher resolutions screens):
image

This is also inline with icons in e.g. the Start Menu or systemtray:

image

Also visually checks out in Figma:
image

@Jay-o-Way Great catch, we probably need to update the icon sizes for the PowerToys flyout as well to 16 instead of 14.

@Jay-o-Way
Copy link
Contributor Author

FontIcon/SymbolIcon uses 20 by default

Could that be, because of a style somewhere, perhaps?

Co-Authored-By: Niels Laute <niels.laute@live.nl>
@niels9001
Copy link
Collaborator

FontIcon/SymbolIcon uses 20 by default

Could that be, because of a style somewhere, perhaps?

Yeah, I'm guessing this might be from the pre-WinUI era where in general icons and fonts were bigger.

@marcelwgn
Copy link
Collaborator

@niels9001 Feel free to merge this if it's fine now (I think you have much more knowledge on this matter than me)

@niels9001
Copy link
Collaborator

niels9001 commented Apr 11, 2023

@Jay-o-Way

Icons seem to be vertically off center now..
image

Edit: looks like it's the link icon. Adding a top margin of 2 on the FontIcon should resolve this?

image

@niels9001 niels9001 self-requested a review April 11, 2023 13:16
@niels9001 niels9001 self-requested a review May 4, 2023 13:54
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.

Thanks

@niels9001 niels9001 merged commit bacd842 into microsoft:main May 4, 2023
@Jay-o-Way Jay-o-Way deleted the fontsize branch May 4, 2023 13:59
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.

None yet

4 participants