Additional SmartHint refactoring #3181
Merged
Conversation
This file contains hidden or 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
Icons still need alignment
Currently only the default FontSize is correctly supported.
The TextBox also uses the "IsHintInFloatingPosition" AP for the prefix text. I did not touch this as it still collapses it when the prefix text is not set.
Also uses the helper text in the 'Smart Hint' demo app for a warning
Fixes failing UI test
nicolaihenriksen
changed the title
Keboo
approved these changes
May 9, 2023
nicolaihenriksen
commented
May 9, 2023
Comment on lines
+24
to
+26
| <converters:ThicknessCloneConverter x:Key="DefaultOrFilledStyleLeadingIconMarginConverterBottom" CloneEdges="Top" FixedRight="6" FixedBottom="-2" /> | ||
| <converters:ThicknessCloneConverter x:Key="DefaultOrFilledStyleTrailingIconMarginConverterTop" CloneEdges="Top" AdditionalOffsetTop="-2" /> | ||
| <converters:ThicknessCloneConverter x:Key="DefaultOrFilledStyleTrailingIconMarginConverterCenter" CloneEdges="Top" AdditionalOffsetTop="-12" /> |
There was a problem hiding this comment.
@Keboo
It's the constants used here (and other similar places) in the FixedXxx and AdditionalOffsetXxx properties that are problematic when a non-default FontSize is applied.
So the correct approach is probably to calculate the values based on the FontSize (if we can manage to come up with the correct equation for it). That would however also require that the converter becomes an IMultiValueConverter which will make the templates even more complex. So I am trying to think of other ways to solve it, but I have yet to come up with a better solution...
Keboo
added
the
release notes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes #3174
A number of issues (including the one above) have been reported with the alignment of the
SmartHint, and there are also several different approaches to "solving the same problem" for the different controls.This PR aims to align this across the control styles for:
TextBoxRichTextBoxPasswordBoxComboBoxDatePickerTimePickerThere are a number of variables to consider when figuring out the hint placement, and I don't believe any of the current controls handle all of them. As mentioned, this PR aims to align the approach across the above templates taking the following into account for the placement of the hint (and leading/trailing icons, buttons, prefix texts, and suffix texts):
Control.PaddingControl.VerticalContentAlignmentControl.HorizontalContentAlignmentControl.HeightControl.FontSize- 🔴 Currently only partially supported; similar to before this PRHintAssist.FloatingScaleHintAssist.FloatingOffsetHintAssist.FloatingHintHorizontalAlignmentHintAssist.FontFamilyThe
Smart Hintdemo app page is extended to include manual testing options for all of this. Please check out the branch and take "this for a spin" playing around with the various options. As noted below, you can configure some combinations which are a bit strange, but I believe you could do that before this PR as well. Additional clean up could be done.In the current state of the PR, the
FontSizeis only supported when no explicitHeightis applied, and even then it is only partially supported if a non-defaultFontSizeis applied. It was like that before this PR as well, although it there may be some differences in certain scenarios; it is a bit hard to say. I am still working on my local branch trying to make this work for all font sizes, but that might end up being a different PR in the end as I think it may require some significant changes to the many converters that are in play for adjustingPadding/Marginin theTop/Center/Bottom/Stretchcases ofVerticalContentAlignment.The
TextBoxstyles support all of the properties that can be modified in these panels, but the other control styles only support a sub-set of them. Each control style seemingly supporting a different sub-set 🤔. This PR does not seek to align that. If people need it, I hope/assume they will create an issue in the repo.It is quite possible for consumers of the controls to configure a combination of the features which end up not looking as nice as you would hope. I believe these are separate issues though, and could be handled as such.
As we've discussed, I believe it might be a good idea to split some of the templates apart going forward. Having a single control template trying to juggle 3 different styles (via attached properties) and placing/moving content accordingly makes the templates quite complex. The changes - in particular added use of converters - in this PR does not make it any less complex!