Skip to content

Conversation

@GID0317
Copy link
Contributor

@GID0317 GID0317 commented Aug 30, 2025

Due to the current absence of Item View #341, the icon is displayed using an item repeater with a button as a template. I have already attempted using GridView, but the performance is terrible, with a 4-second freeze every time a load or search is initiated.

I also noticed that the hyperlink displayed under the settings expander was not automatically themed to use Fluent Design, so i just use customized hyperlink button as work around.

image image image image image

Copy link
Member

@NotYoojun NotYoojun left a comment

Choose a reason for hiding this comment

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

TL;DR: Changes requested before we can merge

Thanks for your contribution! 🙌

We've reviewed your pull request and it's a great step forward.
Before we can merge it, there are just a few things that would make it even better:

Avoid hardcoding icon data in JSON

The icon data is already defined in the icon classes (iNKORE.UI.WPF.Modern.Common.IconKeys).
Instead of duplicating it in JSON, you could use reflection to retrieve the keys and values.
This ensures the data stays in sync and correct without manual duplication.

(One drawback is that the "tags" section would no longer be available - I think that's okay, but we can discuss it here.)

Support multiple icon sets

In iUWM there are several icon sets - not just Segoe Fluent Icons, but also Fluent System Icons, Segoe MDL2, Segoe Icons - and the set can be extended in the future.

I'd suggest adding a ComboBox to select the icon set.

Improve hover/press effects for icon list items

Currently, the icon list item has no hover or press effect in light mode (only in dark mode).
I suggest using a Button control if applicable (since you mentioned GridView has poor performance) to get consistent interaction states.

Use icon data class in examples

In the XAML and C# code example sections, please avoid showing something like:

<ui:FontIcon Glyph="&#xE710;" />

We have an icon data class that simplifies usage. Instead, use:

<ui:FontIcon Icon="{x:Static ui:SegoeFluentIcons.Edit}" />

The same applies to the C# code example.

Match gap value with WinUI Gallery

The spacing between icons in the list doesn't match the WinUI Gallery reference. Please adjust the gap value so it's consistent with the WinUI Gallery's layout.


When you have a chance, please update the PR with these refinements.
If anything is unclear or you'd like guidance, we're more than happy to help - just let us know.

If you don't have the time or energy to make these changes yourself, that's completely okay.
But please say so openly, let us know, and our team or other contributors can step in to help finish it up if anyone is available.

We truly appreciate the time and care you've already put into this.
If you have any objections, questions, or ideas, please reply below so we can work through them together.

@GID0317
Copy link
Contributor Author

GID0317 commented Aug 31, 2025

Hi, thanks you for the PR reviews
I’ll see what I can do by following the recommendations from the list 👍

@GID0317
Copy link
Contributor Author

GID0317 commented Aug 31, 2025

Alright, I think it's working now

We can now use a combo box to control the icon set:

Recording.2025-09-01.024619.mp4

Done list:

  • Avoid hardcoding icon data in JSON (keeping the JSON as a safety fallback for now, but if not needed it can be removed)
  • Support multiple icon sets
  • Use icon data class in examples
  • Match gap value with WinUI Gallery (The code already matches, but the outcome doesn't so I readjusted it)

For the "Improve hover/press effects for icon list items" Sorry I don't know much about this one. I already attempt to fix it but fail
I'm not sure if implementing tags is possible, but if it is, redoing it better could be great. Unfortunately, I just can't handle this one either

Copy link
Member

@NotYoojun NotYoojun left a comment

Choose a reason for hiding this comment

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

Please get rid of these since hardcodings are discouraged. All data retrived by using Reflection

Image

Additionally, maybe removing hardcoding here will make everything better

https://github.com/GID0317/UI.WPF.Modern/blob/3a5eeac9656f8c0201afe0cb7d8de1ec3c831832/source/iNKORE.UI.WPF.Modern.Gallery/Helpers/IconsDataSource.cs#L209-L238

@GID0317
Copy link
Contributor Author

GID0317 commented Sep 1, 2025

I think icondata.json already been removed and is now a no-op, so AvailableSets only contains the sets discovered by reflection in discovery order. For icondata.cs, it's not hardcoded data but a lightweight UI model (DTO) that the Gallery uses to bind to XAML DataTemplates and display icons in the UI. Removing it would break XAML bindings and require significant work, so this is as far as I can go with it

Copy link
Member

@NotYoojun NotYoojun left a comment

Choose a reason for hiding this comment

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

Nice! We're getting closer ever than before. Just a few problems to get rid of:

Incorrect FontFamily applied to FontIcon

Image

When using FluentSystemIcons, icon rendering are clearly problematic. It seems that the FontIcon or TextBlocks are still using FontFamily of SegoeFluentIcons, causing wrong glyph mappings.

Outdated introduction

This needs to be replaced or rewritten as well:

Image

@GID0317
Copy link
Contributor Author

GID0317 commented Sep 1, 2025

Thanks for the hard work!

I think I managed to fix the missing icons sets that appeared square, but it still not fixes some empty icons:
image

Since I couldn't find the documentation (I tried search it on https://docs.inkore.net/en-us/ui-wpf-modern/ but can't find it. I hope there was a search box there in the future), I used the recent changes as references for rewriting the introduction settings expander. However, if there's anything wrong feel free to correct it:
image

@NotYoojun
Copy link
Member

I think I managed to fix the missing icons sets that appeared square, but it still not fixes some empty icons

I've fixed that for you.

Since I couldn't find the documentation (I tried search it on https://docs.inkore.net/en-us/ui-wpf-modern/ but can't find it.

Doc here: https://docs.inkore.net/en-us/ui-wpf-modern/components/media/font-icon

@GID0317
Copy link
Contributor Author

GID0317 commented Sep 3, 2025

Thanks, I've already customized the expander introductions to align with the latest documentary:
image

I also noticed that the font icon change make icons appears dark when using dark mode

<ui:FontIcon FontFamily="{Binding FontFamily}" Foreground="{DynamicResource TextFillColorPrimaryBrush}" Glyph="{Binding Glyph}" HorizontalAlignment="Center" VerticalAlignment="Center"/>
image

When I tried changing it to use a textbox however, it worked great:

<TextBlock FontFamily="{Binding FontFamily}" Foreground="{DynamicResource TextFillColorPrimaryBrush}" Text="{Binding Glyph}" HorizontalAlignment="Center" VerticalAlignment="Center"/> 
image

Should we continue using ui:FontIcon, or should we switch to TextBlock to address this issue?

@NotYoojun
Copy link
Member

The FontIcon foreground issue seems to be related to ShouldInheritForegroundFromVisualParent in IconElement class, and after some investigation I believe its a bug. Unfortunately I don't have that effort to look into it more deeply right now. 😥

@GID0317
Copy link
Contributor Author

GID0317 commented Sep 16, 2025

The FontIcon foreground issue seems to be related to ShouldInheritForegroundFromVisualParent in IconElement class, and after some investigation I believe its a bug. Unfortunately I don't have that effort to look into it more deeply right now. 😥

That's alright, that lead is already useful enough. I'll try to dig deeper into the source code and see what I can do

@GID0317
Copy link
Contributor Author

GID0317 commented Sep 16, 2025

I think #352 Fix this bug, I make the fonticons followed the TextBlock approach by binding the internal _textBlock.Foreground directly to FontIcon.Foreground in InitializeChildren(). It lets WPF handle inheritance automatically when FontIcon.Foreground is default/inherited and it picks up the parent's brush when explicitly set, it uses that value

image image

@NotYoojun NotYoojun merged commit c3af9ec into iNKORE-NET:main Oct 5, 2025
1 check passed
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.

2 participants