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

Smart hint and text box style refactor #3486

Merged
merged 65 commits into from
Apr 18, 2024

Conversation

nicolaihenriksen
Copy link
Contributor

@nicolaihenriksen nicolaihenriksen commented Mar 9, 2024

This PR builds on top of the "RFC" around the TextBox style change: #3461

(Visual) Breaking changes !!!

This PR includes breaking changes.

  • Applying wpf:HintAssist.IsFloating="True" is no longer enough to change from the default style to the floating hint style. Explicit opt-in in the form of Style="{StaticResource MaterialDesignFloatingHintTextBox}" is now required.
    • Although this is a breaking API change, I believe it is a good change. It allows better control to override properties because they are now set in the style rather than in a trigger inside the ControlTemplate.Triggers collection.
  • Minor changes to the TextBox control height in the various styles are to be expected; this also affects some of the UI tests which have been temporarily modified to run green. When all styles adopt the new SmartHint approach, they should be able to run "normally" again.

General idea

image
Instead of a SmartHint consumer relying on a lot of "magic numbers" to place the floating hint correctly, this PR attempts to let the SmartHint handle the placement of the floating hint itself, with a few supporting properties.

In the layout above, the green box represents the PART_ContentHost of the TextBox style which holds the user input. The red box represents the SmartHint as a whole; this element spans the width of the prefix-text, the user input, and the suffix-text in the layout to be able to place float the hint to either align with prefix/suffix, or with the user input. The blue/purple box represents the "scaled hint" inside of the SmartHint.

From a SmartHint consumers point-of-view (e.g. the TextBox style), the idea is that you provide a SmartHint.FloatingTarget, which similarly to the Popup.PlacementTarget is a dependency property where the caller provides a target element which the hint will then (vertically) float on top of. In the example above, the SmartHint.FloatingTarget is set to PART_ContentHost to always float the hint directly on top of the user input. There are a few other supporting properties to enable the hint to horizontally align with either the user input or the prefix/suffix texts; depending on the value of HorizontalContentAlignment as well.

Examples

Align with prefix Align with user input
NonOutlineAllignWithPrefix NonOutlineAllignWithUserInput
OutlineAllignWithPrefix OutlineAllignWithInput

In the - slowed down - animations above, you should note that the whole SmartHint (red box) floats upwards, but only far enough such that the "scaled down hint" (blue/purple box) rests directly on top of the SmartHint.FloatingTarget. Effectively overlapping the user input with the "empty space" left after scaling the hint down. In the outlined style, the SmartHint.FloatingTarget is set as the outer Border, and a few supporting properties have been set to allow the hint to center align (vertically) with the border line.

This approach, in my opinion, works better than the old implementation. The intention is to retain all existing functionality (but improve upon it), and also add support for change of font size and multi-line input. I believe the branch in its current state nearly has that, with just a few minor things I need to look into / clean up.

The intention is to use the TextBox style as the guinea pig for adopting the new approach, and then to roll it out to all the other styles containing a SmartHint afterwards. Backwards compatibility is intended to be maintained, so if a non-default value for HintAssist.FloatingOffset is supplied, that will still be respected. It could however be considered a both a behavioral breaking change (alignment with prefix/suffix/user-input) and a (slight) visual breaking change.

Funkyness

image
This particular (existing) feature is rather funky. I retained back-compat of it, but it is not aligned across the styles at all. It seems like a shortcut instead of doing an actual MD3 version of the styles. It basically changes the default TextBox style so that the underline behaves slightly different on hover.

Any comments/feedback/pushback is welcome.

@Keboo Keboo added this to the 5.1.0 milestone Mar 10, 2024
@Keboo Keboo added enhancement release notes Items are likely to be highlighted in the release notes. labels Mar 10, 2024
nicolaihenriksen and others added 26 commits March 28, 2024 13:45
(cherry picked from commit 555d6f9979cb32e004ceb02a716d8fe11ec3d601)
(cherry picked from commit 3b61edd)
@nicolaihenriksen nicolaihenriksen force-pushed the smartHintAndTextBoxStyleRefactor branch from 77065f0 to 9f9c924 Compare March 28, 2024 14:13
@nicolaihenriksen nicolaihenriksen added visual breaking change Items here have affected the visual look of controls. breaking change Items here have breaking API changes. labels Mar 30, 2024
@nicolaihenriksen nicolaihenriksen marked this pull request as ready for review March 30, 2024 14:48
@nicolaihenriksen nicolaihenriksen requested a review from Keboo March 30, 2024 14:48
</StackPanel>");

var height = await GetHeight(stackPanel, "TextBox");
//Assert.Equal(height, await GetHeight(stackPanel, "ComboBox"), Precision);
Copy link
Contributor Author

@nicolaihenriksen nicolaihenriksen Apr 7, 2024

Choose a reason for hiding this comment

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

@Keboo FYI. Look at the file as a whole. Basically I have made a duplicate test for each of the existing tests in this class in order to migrate the controls one by one as they adopt the new SmartHint approach. The difference in height is typically around 0.5-1 pixels, but the tests expect the height to match down to the third decimal; that does not work and therefore I am, temporarily, introducing these "overloads".

In the other PR's adopting the new SmartHint you will see controls move from the original test method into the Xxx_PostSmartHintRefactor() "overload" method (one control per PR). An example is the ComboBox, where you can see this happen:
https://github.com/MaterialDesignInXAML/MaterialDesignInXamlToolkit/pull/3515/files#diff-e858ebe6ad71bec638332cb3f89c65f1570e5a3bcf92985e967c1ccb74f094d3

Once all styles have adopted the new approach, we can collapse into a single test with the original name again.

Also, please don't convert the XAML input to raw string literals yet; I will do a post-adoption PR where I clean up some of the leftovers like that along with collapsing methods, removing temporary classes, etc....

@nicolaihenriksen nicolaihenriksen removed the breaking change Items here have breaking API changes. label Apr 18, 2024
@nicolaihenriksen nicolaihenriksen merged commit 7f27b72 into master Apr 18, 2024
2 checks passed
@nicolaihenriksen nicolaihenriksen deleted the smartHintAndTextBoxStyleRefactor branch April 18, 2024 18:18
adrbarros added a commit to adrbarros/MaterialDesignInXamlToolkit that referenced this pull request Apr 19, 2024
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. visual breaking change Items here have affected the visual look of controls.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants