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

Added support for limiting max tokens in TokenizingTextBox #4163

Merged
merged 16 commits into from
Aug 13, 2021

Conversation

shweaver-MSFT
Copy link
Member

@shweaver-MSFT shweaver-MSFT commented Aug 4, 2021

Related to #3672

This PR adds support for a new property, TokenizingTextBox.TokenSelectionMode, which allows a developer to specify if the TTB should allow one or many tokens inside the box as results.

This PR adds support for a new property, TokenizingTextBox.MaxTokens, which allows a developer to specify a maximum number of token results allowed in the TTB.

An issue was originally reported on the Graph-Controls repo mentioning that the SelectionMode property is not being respected in the PeoplePicker control. The use case was to use PeoplePicker to select a person for a particular role in which there should only be one person assigned. Without a specific property to support this, applying a workaround for restricting the TokenizingTextBox/PeoplePicker controls to only allow one token is a pain.

That said, SelectionMode actually comes from ListViewBase, and is more associated with "Selection" in a different context. Work could be done to enable other "selection" features for tokens, but that's not what this issue is really about.

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

Currently the TokenizingTextBox is allows multiple tokens to be selected and there is no mechanism for restricting the box to only one token at a time.

Meanwhile, the SelectionMode property exists but is otherwise unused in this control. It's a separate thing.

What is the new behavior?

The new TokenSelectionMode enum and TokenizingTextBox.TokenSelectionMode property controls how many tokens can be present in the TTB at a time.

The default is TokenSelectionMode.Multiple so that it works the same out of the box as before. However, you can now specify TokenSelectionMode.Single to restrict the TTB to only one token. In single mode, adding a new token will replace any existing.

TokenSelectionMode only has two values, Single and Multiple.

The new TokenizingTextBox.MaximumTokens property controls how many tokens can be present in the TTB at a time.

Potential values:

  • The default is null, meaning any number of tokens can be added without restriction.
  • A value of 0 or less makes the control fairly useless. You can search for results, but not add them. Perhaps an opportunity to throw an exception? I chose to simply handle the value as best as possible.
  • Any other positive integer value will limit the maximum number of tokens allowed in the control as expected.

A counter visual has been added at the end to indicate how many items are present and the maximum number of tokens allowed. The counter visual is only visible when the MaximumTokensProperty is greater than 0.

image

When at maximum token capacity, the control will not accept new items and the counter will turn red:
image

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Pull Request has been submitted to the documentation repository instructions. Link:
  • Sample in sample app has been added / updated (for bug fixes / features)
  • New major technical changes in the toolkit have or will be added to the Wiki e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
  • Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Header has been added to all new source files (run build/UpdateHeaders.bat)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Aug 4, 2021

Thanks shweaver-MSFT for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

@ghost ghost requested review from michael-hawker, azchohfi and Rosuavio August 4, 2021 22:52
@ghost ghost added bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️ labels Aug 4, 2021
@shweaver-MSFT
Copy link
Member Author

shweaver-MSFT commented Aug 4, 2021

As soon as I posted this, I had a thought... What if this feature were a boolean instead of an enum? But what to call it, IsMultiSelectionEnabled="False" or just MultiSelect="False"? I'm not sure. Open to suggestions!

@michael-hawker
Copy link
Member

Yeah, converting to draft while we figure these questions out. 🙂

Selection is kind of overloaded in this way as well (as we have 'selecting' a token which puts it in a list which can then be 'selected' later (which is what the ListView property is supposed to affect)). If we add this, I definitely think we need a different property, and maybe later we support the actual SelectionMode as well for a user selecting tokens (we do support selection of multiple tokens with Ctrl and stuff I think, so I don't think that should be too hard). I think I like TokenSelectionMode over a generalized name.

I also just don't know if it makes sense to have this mode on this control vs. just using a ComboBox. Like is it better for us to have a sample using a GraphPresenter and a ComboBox (probably a cool sample to build for validation later anyway) or make a new control vs. adding a mode to the TokenizingTextBox?

However, I do like the idea of MaximumItems (or MaximumTokens) instead which supports more scenarios. Though I think we have to think about the single selection case, as if you then go select someone else, you'd probably want to replace the person you have vs. keeping the one you have in there. So does this apply to more than one as well, like it'd be a FIFO sort of thing?

@RosarioPulella @XAML-Knight would love to know your thoughts too.

FYI @marcpems

@XAML-Knight
Copy link
Contributor

I like this PR as-is, and view an added MaximumTokens (or even MaximumSelectedTokens) property as a nice-to-have.

With regards to converting this control over to ComboBox, not sure what value-add that gives our users, and wouldn't want to Swiss-Army-Knife the TTB. Better to create a new control.

@shweaver-MSFT
Copy link
Member Author

I really like the MaximumTokens idea! But it does introduce some interesting scenarios to handle when inserting tokens in between other tokens when at the maximum limit. What do we do with the new token? I think it could make sense to replace whatever the following token is; e.g. A new token at the beginning should replace the first preexisting token.

I also like how the two could be complimentary, TokenSelectionMode and MaximumTokens. I only really need TokenSelectionMode at the moment, so maybe this PR is fine after all, and we can create an issue to track the MaximumTokens feature.

@michael-hawker, what do you think?

@michael-hawker
Copy link
Member

I do think that if we had both it could be a bit confusing, as if TokenSelectionMode was Single, then that would supersede any value for the MaximumTokens property. While MaximumTokens does a bit more, I do think it handles the initial scenario by construction as well, especially as the UX of maxing sure we do something when the user interacts should be similar scenarios.

@shweaver-MSFT
Copy link
Member Author

I think you're right @michael-hawker, MaximumTokens kind of covers all cases already. TokenSelectionMode would end up being redundant or conflicting in some cases.

I'll change it to MaximumTokens and when adding a token while the max token limit is already hit, the new token will replace the following token. If the token is added at the end, it will replace the last existing token.

Sound good?

@shweaver-MSFT shweaver-MSFT changed the title Added support for TokenSelectionMode in TokenizingTextBox Added support for limiting max tokens in TokenizingTextBox Aug 6, 2021
…nizingTextBox.Properties.cs

Co-authored-by: Michael Hawker MSFT (XAML Llama) <[email protected]>
@ghost
Copy link

ghost commented Aug 6, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@ghost
Copy link

ghost commented Aug 9, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

Copy link
Contributor

@XAML-Knight XAML-Knight left a comment

Choose a reason for hiding this comment

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

After the recent changes, I can sometimes add more tokens than should be allowed (in this case, 4 tokens added when MaxTokens is 3):
image

@ghost
Copy link

ghost commented Aug 10, 2021

This PR has been marked as "needs attention 👋" and awaiting a response from the team.

@shweaver-MSFT
Copy link
Member Author

@XAML-Knight you are right, my previous "bugfix" commit just added more bugs! 🐛🐜 That's what I get for moving too fast. Should be all fixed up now.

tokenBox.AddTokenItem("TokenItem3");

Assert.AreEqual(startingItemsCount + maxTokens, tokenBox.Items.Count, "Token Replace failed");
Assert.AreEqual("TokenItem1", tokenBox.Items[0]);
Copy link
Member

Choose a reason for hiding this comment

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

Weren't we expecting this to be TokenItem2 as TokenItem1 was the oldest?

Copy link
Member Author

Choose a reason for hiding this comment

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

TokenItem2 is the most recently added (right-most), so it gets replaced when TokenItem3 is added because MaxTokens is 2. TokenItem1 is the oldest (we added it first), so it is the left-most token and remains unaffected.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I thought we had discussed a FIFO approach vs. a LIFO. Want me to poll our community to get a litmus test? I can generalize it to try and be more objective like:

If you had a collection of elements [1, 2, 3] with a maximum size (3), but had to add a new item at the end ("4"). Which of the following would you expect?

  1. Oldest Item is Removed: List is now [2, 3, 4]
  2. Newest Item is Removed: List is now [1, 2, 4]
  3. Something else?

(Because I think we at least agreed we should do something over nothing, eh?)

Copy link
Member

Choose a reason for hiding this comment

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

Reached out to the UWP Community Discord and asked question here (with more options about inserting): https://discord.com/channels/372137812037730304/663434534087426129/875096129828708412

General consensus was adding a new item should be prevented and we should probably just indicate in the box the count of items in the limit:

image

(mock up in sample app overlaid a new TextBlock over the TokenizingTextBox (by wrapping in a Grid))

<TextBlock Text="2/3" HorizontalAlignment="Right" VerticalAlignment="Center" Margin="0,0,32,2"/>

(though not sure how that'd display if it goes multi-line... and we can probably arrange better in actual control template of course, but wanted to provide visual)

P2: Turn red when at max.
P2: Behavior over-ridable by dev (I'm wondering if the add event still fires and the dev could potentially remove a token of their choosing before the new one would check the max... that way they could implement any of the behaviors we've discussed with the insertion behavior...).

Copy link
Member Author

Choose a reason for hiding this comment

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

I love it! I also think it is so cool that we can simply poll the community on the fly and get feedback! I'll work on adding the counter :) The P2s look interesting too. I can probably get the red-at-max feature handled, but the max-item-behavior is a bit more work. I'll create an issue so we won't forget.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh also, I am still planning on reducing the number of tokens from the end if the MaxTokens value is reduced below the current count. However, I don't expect changing the MaxTokens value on the fly to be a common scenario.

@shweaver-MSFT
Copy link
Member Author

Build is complaining about the System.Math call...

 D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls.Input\TokenizingTextBox\TokenizingTextBox.cs(85,80): error CS0103: The name 'Math' does not exist in the current context [D:\a\1\s\Microsoft.Toolkit.Uwp.UI.Controls.Input\Microsoft.Toolkit.Uwp.UI.Controls.Input.csproj]

But it definitely exists. The code build and deploys fine locally. What gives?

@michael-hawker
Copy link
Member

LGTM! 🎉🎉🎉

@ghost ghost removed the needs attention 👋 label Aug 13, 2021
@michael-hawker michael-hawker merged commit 933a1c6 into main Aug 13, 2021
@delete-merged-branch delete-merged-branch bot deleted the shweaver/token-selection-mode branch August 13, 2021 23:08
@michael-hawker michael-hawker added this to the 7.1 milestone Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An unexpected issue that highlights incorrect behavior controls 🎛️
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants