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

Remove MDL2 from code #1260

Merged
merged 2 commits into from
May 4, 2023
Merged

Remove MDL2 from code #1260

merged 2 commits into from
May 4, 2023

Conversation

Jay-o-Way
Copy link
Contributor

@Jay-o-Way Jay-o-Way commented Apr 10, 2023

Description

Let's not use MDL2 :)

Motivation and Context

Closes: #1252

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

@tashatitova tashatitova left a comment

Choose a reason for hiding this comment

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

We can't just remove it like that. We use SymbolThemeFontFamily that redirects to appropriate font for controls that use icons. Otherwise it will fall back to Xaml Auto Font Family which is a text font. Also, all these instances would need to be tested, for example, AnimatedIcon

@Jay-o-Way
Copy link
Contributor Author

Jay-o-Way commented Apr 10, 2023

@tashatitova that's highly peculiar because this very app shows examples just like this. <Fonticon glyph="">

Also (as mentioned in the issue) it renders just fine.

Can you show how it looks? Need some convincing here.

@tashatitova
Copy link

FontIcon defaults to SymbolThemeFontFamily, but usage without setting the FontFamily isn't something recommended, at least as far as I've seen. Textblocks do come without font sent on them, FontIcons usually specify it.

@Jay-o-Way
Copy link
Contributor Author

Can you show how it looks on your device?

@Jay-o-Way
Copy link
Contributor Author

Jay-o-Way commented Apr 12, 2023

@tashatitova

FontIcon defaults to SymbolThemeFontFamily

This speaks for itself. It's 100% redundant.

If you don't specify a FontFamily, or you specify a FontFamily that is not available on the system at runtime, the FontIcon falls back to the default font family defined by the SymbolThemeFontFamily theme resource.

By default, Windows uses the Segoe Fluent Icon font family. If your app is run on Windows 10, version 20H2 or earlier, the Segoe Fluent Icon font family is not available and the SymbolThemeFontFamily resource falls back to the Segoe MDL2 Asset font family instead. To use glyphs from the default system font, don't set the FontFamily property, let it use its default value.

https://learn.microsoft.com/windows/windows-app-sdk/api/winrt/microsoft.ui.xaml.controls.fonticon?view=windows-app-sdk-1.2#default-font-family

isn't something recommended, at least as far as I've seen

Have a look at WinUI 3 Gallery, it's a good source:

<DropDownButton AutomationProperties.Name="Email">
    <DropDownButton.Content>
        <FontIcon Glyph="&#xE715;"/>
    </DropDownButton.Content>
    <DropDownButton.Flyout>
        <MenuFlyout Placement="Bottom">
            <MenuFlyoutItem Text="Send">
                <MenuFlyoutItem.Icon>
                    <FontIcon Glyph="&#xE725;"/>
                </MenuFlyoutItem.Icon>
            </MenuFlyoutItem>
            <MenuFlyoutItem Text="Reply">
                <MenuFlyoutItem.Icon>
                    <FontIcon Glyph="&#xE8CA;"/>
                </MenuFlyoutItem.Icon>

@Jay-o-Way
Copy link
Contributor Author

@niels9001 I'm ready

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, changes look good.

@niels9001 niels9001 requested a review from tashatitova May 4, 2023 13:56
@niels9001 niels9001 dismissed tashatitova’s stale review May 4, 2023 13:56

FontIcon falls back automatically to SymbolThemeFontFamily

@niels9001 niels9001 merged commit 4be8814 into microsoft:main May 4, 2023
@Jay-o-Way Jay-o-Way deleted the Remove-MDL2 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.

XAML code still declares Font Family "MDL2"
4 participants