-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
+177
−16
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
fafbf5b
Added support for TokenSelectionMode in TokenizingTextBox
shweaver-MSFT fbeb2c4
Added TokenItemRemoved events
shweaver-MSFT 93ca18a
Replaced TokenSelectionMode with MaxTokens property on TokenizingTextBox
shweaver-MSFT 9ee8b96
Removed TokenSelectionMode enum
shweaver-MSFT b4fecf3
Accounting for negative MaxToken value
shweaver-MSFT 358ada4
Added MaxTokens test
shweaver-MSFT f0d8b2a
Made MaxTokens non-nullable and added checks for DP.UnsetValue
shweaver-MSFT 1309ca1
Update Microsoft.Toolkit.Uwp.UI.Controls.Input/TokenizingTextBox/Toke…
shweaver-MSFT c7b55c6
Updated OnMaxTokensChanged method and improved MaxTokens test
shweaver-MSFT 07f19d5
Bug fixes
shweaver-MSFT 01c413b
bugfix bug fixes
shweaver-MSFT a7bb51b
Added binding to MaxTokens value in sample
shweaver-MSFT c3903ab
Updated TokenizingTextBox.MaxTokens test
shweaver-MSFT 86cd4d6
Updated Max to Maximum and added token counter
shweaver-MSFT 2d118e8
Updated GetValue to ReadLocalValue
shweaver-MSFT ccefe38
Removing Math reference
shweaver-MSFT File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
(Because I think we at least agreed we should do something over nothing, eh?)
There was a problem hiding this comment.
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:
(mock up in sample app overlaid a new TextBlock over the TokenizingTextBox (by wrapping in a Grid))
(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...).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.