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

Minor UX tweaks for icons + color sections #1725

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

niels9001
Copy link
Contributor

@niels9001 niels9001 commented Jan 13, 2025

This PR introduces the following changes:

  • Tweaks the UX of the Icon panel for better readability.
  • Using an ItemsView without buttons, for better accessibility. Using a WrapLayout (from the WCT) so labels are not cut off.
  • Selecting the first item upon loading, or after searching.
  • Collapse the sidepanel until the grid icons is loaded: current behavior shows empty content on the right for 2 seconds which does not look great.

image

  • Spacing tweaks for the color sections so they are clustered together a bit better
  • Using a 0.8 opacity for the subtitle to be a bit less prominent

image

@niels9001 niels9001 requested a review from marcelwgn January 13, 2025 17:56
@Zakariathr22
Copy link
Contributor

Hi @niels9001,

I believe there's a small issue. The actual tags for the Lock icon are:

{
  "Code": "E72E",
  "Name": "Lock",
  "Tags": [
    "private",
    "security",
    "access",
    "protected",
    "safety",
    "permission",
    "access",
    "protection"
  ]
}

However, the displayed tags don't match. It seems the app is showing the initial tags from the previously selected icon and then combining them with the remaining tags from the currently selected icon (you can compare the main branch with the new branch to observe the difference).

image

Copy link
Contributor

@Zakariathr22 Zakariathr22 left a comment

Choose a reason for hiding this comment

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

@niels9001

It seems the issue arises from the WrapLayout not correctly tracking changes in the items. This is what result in incorrect tags updates when the selected icon changes.

I suggest modifying the IconsItemsView_SelectionChanged method as follows:

private void IconsItemsView_SelectionChanged(ItemsView sender, ItemsViewSelectionChangedEventArgs args)
{
    if (IconsItemsView.ItemsSource is IList<IconData> currentItems)
    {
        if (IconsItemsView.CurrentItemIndex != -1 && IconsItemsView.CurrentItemIndex < currentItems.Count)
        {
            SelectedItem = currentItems[IconsItemsView.CurrentItemIndex];
        }
    }

    if (TagsItemsView != null)
    {
        TagsItemsView.Layout = new WrapLayout { VerticalSpacing = 4, HorizontalSpacing = 4 };
    }
}

This ensures that the layout for TagsItemsView is refreshed whenever the selection changes. By reassigning a new instance of WrapLayout with the same spacing values (VerticalSpacing = 4, HorizontalSpacing = 4), we effectively reset the layout. This forces the ItemsRepeater to re-measure and re-arrange the items, ensuring it reflects the current data accurately.

@niels9001 niels9001 marked this pull request as draft January 14, 2025 09:03
@niels9001
Copy link
Contributor Author

@niels9001

It seems the issue arises from the WrapLayout not correctly tracking changes in the items. This is what result in incorrect tags updates when the selected icon changes.

I suggest modifying the IconsItemsView_SelectionChanged method as follows:

private void IconsItemsView_SelectionChanged(ItemsView sender, ItemsViewSelectionChangedEventArgs args)
{
    if (IconsItemsView.ItemsSource is IList<IconData> currentItems)
    {
        if (IconsItemsView.CurrentItemIndex != -1 && IconsItemsView.CurrentItemIndex < currentItems.Count)
        {
            SelectedItem = currentItems[IconsItemsView.CurrentItemIndex];
        }
    }

    if (TagsItemsView != null)
    {
        TagsItemsView.Layout = new WrapLayout { VerticalSpacing = 4, HorizontalSpacing = 4 };
    }
}

This ensures that the layout for TagsItemsView is refreshed whenever the selection changes. By reassigning a new instance of WrapLayout with the same spacing values (VerticalSpacing = 4, HorizontalSpacing = 4), we effectively reset the layout. This forces the ItemsRepeater to re-measure and re-arrange the items, ensuring it reflects the current data accurately.

Thanks.. something to follow up on with the Toolkit! Your workaround seems to work, thanks for the suggestion 😄!

@niels9001 niels9001 marked this pull request as ready for review January 14, 2025 10:26
<StackPanel>
<FontIcon
Margin="0,12,0,24"
Copy link
Contributor

@Zakariathr22 Zakariathr22 Jan 14, 2025

Choose a reason for hiding this comment

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

IMHO, we should keep the top margin 12 to ensure equal margins at the top and bottom? (like in the following pic)
image

<toolkit:SettingsCard
Padding="16,8"
AutomationProperties.Name="How to use the font"
ContentAlignment="Left">
<StackPanel Orientation="Vertical" Spacing="8">
<RichTextBlock TextWrapping="Wrap">
Copy link
Contributor

Choose a reason for hiding this comment

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

My last thought in this PR is that the paragraphs are too close together, making it hard to tell whether each part is a separate idea or connected to the previous one.

Wouldn't adding some space between paragraphs (margins) improve readability and clarity?

image

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