Mhawker/ttb updates #3319
Mhawker/ttb updates #3319
Conversation
Ref microsoft/microsoft-ui-xaml#2568 Also added WPF TryFindResource method to LogicalTree helpers - thanks for review/assist @rudyhuyn
|
Thanks michael-hawker 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
assigned
| <Thickness x:Key="TokenizingTextBoxPresenterMargin">0,0,6,0</Thickness> | ||
| <x:Double x:Key="TokenizingTextBoxTokenSpacing">2</x:Double> | ||
| <Thickness x:Key="TextControlBorderThemeThicknessFocused">2</Thickness> | ||
| <x:Double x:Key="TokenizingTextBoxIconFontSize">16</x:Double> |
There was a problem hiding this comment.
This is actually ignored as we don't have a good way to access it from the code-behind of the templated control (another helper in the future?). I'm not sure if that's what the ComponentResourceKey was for in WPF (I never used that in my days there).
I can add a comment linking to the hard-coded value on line 110 below in TTB.ASB.cs.
…or initial value of TokenizingTextBoxItem
|
Ugh, this might be breaking something when adding a token, investigating... |
This was an issue with the PlaceholderText ContentControl in the AutoSuggestBox which would not update it's layout properly when the ASB had focus when we would add/remove a token.
marcpems
left a comment
marcpems
left a comment
There was a problem hiding this comment.
LGTM, only minor comments
| placeholder.Visibility = Visibility.Collapsed; | ||
|
|
||
| // After we ensure we've hid the control, make it visible again (this is inperceptable to the user). | ||
| _ = CompositionTargetHelper.ExecuteAfterCompositionRenderingAsync(() => |
Microsoft.Toolkit.Uwp.UI.Controls/TokenizingTextBox/TokenizingTextBox.xaml
Show resolved
Hide resolved
| { | ||
| if (current.Resources?.TryGetValue(resourceKey, out value) == true) | ||
| { | ||
| return value; |
There was a problem hiding this comment.
I'm not keen on multiple return points from a function as it can make maintenance more tricky.
however this looks pretty simple and easy to read in this case.
Microsoft.Toolkit.Uwp.UI.Controls/TokenizingTextBox/TokenizingTextBox.cs
Show resolved
Hide resolved
| /// <returns>Awaitable Task</returns> | ||
| public static Task<bool> ExecuteAfterCompositionRenderingAsync(Action action) | ||
| { | ||
| Guard.IsNotNull(action, nameof(action)); |
There was a problem hiding this comment.
Could/should this just be a no-op (Task.CompletedTask) in this case? Do we want to throw?
| /// </summary> | ||
| /// <param name="action">Action to be executed after render pass</param> | ||
| /// <returns>Awaitable Task</returns> | ||
| public static Task<bool> ExecuteAfterCompositionRenderingAsync(Action action) |
There was a problem hiding this comment.
I would like a Task based version (for the action parameter, Func) so the returned task from this method can await for the action to executed. Not sure its part of this PR, just something I noticed when I was reviewing this.
5 participants
Related #3289
TryFindResource(from WPF) compatible functionUse existingread-onlyParentproperty instead of newOwnerfor TTBIPR Type
What kind of change does this PR introduce?
What is the current behavior?
Layout was incorrect when ASB had focus, and had no text, and added/removed token
FontSize had to be manually set
What is the new behavior?
We now toggle the visibility of the offending PlaceholderText ContentControl which resolves the issue for now.
Now we only set FontSize to our desired size if it's not overwritten by the developer. It can also be overridden by a style property.
I have my resource which the lower box icon is inheriting properly, the top example is overriding it with a local value.
PR Checklist
Please check if your PR fulfills the following requirements:
Other information