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

[Feature] Add AutoSuggestion option for TextBox #3207

Merged

Conversation

AbderrahmaneAhmam
Copy link
Contributor

Description:

Hello everyone,

This pull request introduces a new feature to the MaterialDesignInXamlToolkit project, enabling the AutoSuggestion option for the TextBox control. This feature enhances user experience by providing automatic suggestions as the user types. The search algorithm is controlled by the developer in the ViewModel part, making it customizable and flexible.

Key Changes:

  • Added AutoSuggestionEnabled property to the TextFieldAssist.
  • Added AutoSuggestionItemsSource property to the TextFieldAssist.
  • Added AutoSuggestionItemTemplate property to the TextFieldAssist.
  • Added AutoSuggestionItemTemplateSelector property to the TextFieldAssist.
  • Added AutoSuggestionValueMember property to the TextFieldAssist.
  • Added AutoSuggestionDisplayMember property to the TextFieldAssist.

This contribution aligns with the MaterialDesignInXamlToolkit project's goals of providing modern and user-friendly UI components. As a first-time contributor to an open source project, I am thrilled to be a part of this community and contribute to its growth. I would greatly appreciate any feedback and guidance to improve the feature and align it with the project's standards.

I have thoroughly tested the feature and ensured it seamlessly integrates with the existing codebase. I'm excited about this contribution and believe it will be valuable for the project's users.

I'm looking forward to your feedback and the opportunity to collaborate on further enhancing this feature.

Please let me know if there are any remarks or changes you would like me to make.

Screenshots & Demo

Demo_AutoSuggestion_AdobeExpress

@Keboo Keboo added this to the 4.10.0 milestone May 21, 2023
@Keboo Keboo added enhancement release notes Items are likely to be highlighted in the release notes. labels May 21, 2023
@Keboo Keboo self-requested a review May 21, 2023 19:01
Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

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

Hi @AbderrahmaneAhmam

Thank you for this. I think this would be a nice addition as there have been requests for something similar in the past.

A few suggestions before we merge it.

  1. I think this would be better as a new control that derives from TextBox, rather than adding a bunch of attached properties. This would move all of the attached properties into dependency properties on the new control.
  2. This will likely mean duplicating the existing TextBox Styles/Templates to target the new control type.
  3. I think doing this would allow for a cleaner setup. The needed setup right now needing the register for the TextChanged to show the drop down is a bit awkward to use. With a custom control, it would be able to register its own TextChanged event handler to manage showing the list.

Again, good work so far.

Comment on lines 93 to 119
private ObservableCollection<string> listSuggestion;

public ObservableCollection<string> ListSuggestion
{
get { return listSuggestion; }
set { SetProperty(ref listSuggestion, value); }
}


private ObservableCollection<string> baseListSuggestion;

public ObservableCollection<string> BaseListSuggestion
{
get { return baseListSuggestion; }
set { SetProperty(ref baseListSuggestion, value); }
}

private ObservableCollection<KeyValuePair<string, Brush>> listColors;

public ObservableCollection<KeyValuePair<string, Brush>> ListColors
{
get { return listColors; }
set { SetProperty(ref listColors, value); }
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: the other fields are named with an underscore prefix.

Suggested change
private ObservableCollection<string> listSuggestion;
public ObservableCollection<string> ListSuggestion
{
get { return listSuggestion; }
set { SetProperty(ref listSuggestion, value); }
}
private ObservableCollection<string> baseListSuggestion;
public ObservableCollection<string> BaseListSuggestion
{
get { return baseListSuggestion; }
set { SetProperty(ref baseListSuggestion, value); }
}
private ObservableCollection<KeyValuePair<string, Brush>> listColors;
public ObservableCollection<KeyValuePair<string, Brush>> ListColors
{
get { return listColors; }
set { SetProperty(ref listColors, value); }
}
private ObservableCollection<string> _listSuggestion;
public ObservableCollection<string> ListSuggestion
{
get { return _listSuggestion; }
set { SetProperty(ref _listSuggestion, value); }
}
private ObservableCollection<string> _baseListSuggestion;
public ObservableCollection<string> BaseListSuggestion
{
get { return _baseListSuggestion; }
set { SetProperty(ref _baseListSuggestion, value); }
}
private ObservableCollection<KeyValuePair<string, Brush>> _listColors;
public ObservableCollection<KeyValuePair<string, Brush>> ListColors
{
get { return _listColors; }
set { SetProperty(ref _listColors, value); }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your reply.
Is it acceptable to duplicate all the styles of the TextBox to be reused in the custom control? If so, please provide a suggestion for the name of the custom control. Additionally, for the TextChanged event, I propose extracting it as a command. This approach allows for better control of the search algorithm through the ViewModel.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, in this case I think it is better to duplicate the styles. Though it may seem wasteful within this library, I have been wanting to reduce the size of the templates as they can bloat the size of the visual tree when users are not leveraging all of the features contained within them.

For the name I lets mirror the WinUI/UWP naming (since there doesn't seem to be consensus for material design) and use the name "AutoSuggestBox"

Extracting it as a command is fine in many cases. But am not sure it provides much value in this case. I say this because binding to the Text property within the view model can get the same behavior. Consider the following:

<materialDesign:AutoSuggestBox Test="{Binding MyText, UpdateSourceTrigger=PropertyChanged}" Suggestions="{Binding MySuggestions}" />

Then with this view model:

public class MyViewModel
{
    //... INotifyPropertyChanged implementation

    private string _myText;
    public string MyText
    {
        get => _myText;
        set
        {
            if (value != _myText)
            {
                _myText = value;
                MySuggestions = originalList.Where(x => IsMatch(x, value)).ToList();
                //... INPC
            }
        }
    }

    private IList<string> _mySuggestions;
    public IList<string> MySuggestions
    {
        get => _mySuggestions;
        set
        {
            if (value != _mySuggestions)
            {
                _mySuggestions = value;
                //... INPC
            }
        }
    }

    private static bool IsMatch(string item, string currentText)
    {
        //TODO Custom filter logic
    }
}

As an alternative by simply having the control rely on bindings it also enables filtering with an ICollectionView. The above view model could then be reimplemented like this to leverage the the ICollectionView.Filter:

public class MyViewModel
{
    //... INotifyPropertyChanged implementation

    private string _myText;
    public string MyText
    {
        get => _myText;
        set
        {
            if (value != _myText)
            {
                _myText = value;
                MySuggestions.Filter = x => IsMatch(x, value);
                //... INPC
            }
        }
    }

    private readonly ObservableCollection<string> _suggestions;
    public ICollectionView MySuggestions { get; }
    
    public MyViewModel()
    {
        _suggestions = new();
        MySuggestions = CollectionViewSource.GetDefaultView(_suggestions);
    }

    private static bool IsMatch(string item, string currentText)
    {
        //TODO Custom filter logic
    }
}

It is also worth noting that even with this setup, it is still perfectly possible for someone to decide to bind the command and set it up as you did. For this library we trying to give people the freedom to set things up as they want without being prescriptive on one approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Keboo. That's clear. I will start working on this task and try to finish it as soon as possible.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to reach out if you have questions. I really appreciate the help and look forward to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @Keboo ,

I have implemented this approach. Could you please verify if any adjustments or modifications are needed? I have not yet completed the testing phase, and for testing purposes, I have used the same file "Fields.Xaml". I am unsure if we need to create a separate page for this new control.

Thank you for your assistance.

@Keboo Keboo modified the milestones: 4.10.0, 5.0.0 Jul 19, 2023
@AbderrahmaneAhmam
Copy link
Contributor Author

Hello @Keboo,

I am currently working on the TODOs, but I am having difficulty understanding this specific part:
"TODO: Test case with ICollectionSource." Could you please provide some clarification?

@Keboo
Copy link
Member

Keboo commented Aug 13, 2023

Hello @Keboo,

I am currently working on the TODOs, but I am having difficulty understanding this specific part: "TODO: Test case with ICollectionSource." Could you please provide some clarification?

Great. Sorry for just jumping in and doing some work on it, kinda got a little over excited and I knew my recent file-scope namespace all the things was going to cause conflicts.

For the ICollectionView I have pushed another commit that shows what I was thinking (the test is currently failing).
I suspect that to get the selection working, we may end up needing to make the base class Selector and then add additional properties to support the normal TextBox behavior (In this case I think the behavior of this control may be closer to a ComboBox than a TextBox).

@AbderrahmaneAhmam
Copy link
Contributor Author

Hi @Keboo,

sorry, I understand now what you are referring to with "IsSynchronizedWithCurrentItem". To be honest, I haven't used it before, but after doing some research, I've gained a better understanding. I have a suggestion: how about enabling it by default in the ListBox of AutoSuggestBox? This would allow us to use it for keyboard navigation through ICollectionView. Additionally, this option would be available for consumers as well.

@Keboo
Copy link
Member

Keboo commented Aug 16, 2023

Hi @Keboo,

sorry, I understand now what you are referring to with "IsSynchronizedWithCurrentItem". To be honest, I haven't used it before, but after doing some research, I've gained a better understanding. I have a suggestion: how about enabling it by default in the ListBox of AutoSuggestBox? This would allow us to use it for keyboard navigation through ICollectionView. Additionally, this option would be available for consumers as well.

I think that would be a good idea.

@anno0
Copy link

anno0 commented Sep 5, 2023

Will it be possible to use it in a RichTextBox?

@AbderrahmaneAhmam
Copy link
Contributor Author

Will it be possible to use it in a RichTextBox?

Actually, this pull request is not related to the RichTextBox; it's about creating a new control, the AutoSuggestBox.

@Keboo Keboo merged commit 9be7c3d into MaterialDesignInXAML:master Sep 21, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release notes Items are likely to be highlighted in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants