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

Additional SmartHint refactoring #3181

Merged
merged 25 commits into from
May 11, 2023
Merged

Additional SmartHint refactoring #3181

merged 25 commits into from
May 11, 2023

Conversation

nicolaihenriksen
Copy link
Contributor

@nicolaihenriksen nicolaihenriksen commented May 1, 2023

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:

  • TextBox
  • RichTextBox
  • PasswordBox
  • ComboBox
  • DatePicker
  • TimePicker

There 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.Padding
  • Control.VerticalContentAlignment
  • Control.HorizontalContentAlignment
  • Control.Height
  • Control.FontSize - 🔴 Currently only partially supported; similar to before this PR
  • HintAssist.FloatingScale
  • HintAssist.FloatingOffset
  • HintAssist.FloatingHintHorizontalAlignment
  • HintAssist.FontFamily
  • Styles ("Floating", "Filled", "Outlined")

The Smart Hint demo 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.

image

In the current state of the PR, the FontSize is only supported when no explicit Height is applied, and even then it is only partially supported if a non-default FontSize is 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 adjusting Padding/Margin in the Top/Center/Bottom/Stretch cases of VerticalContentAlignment.

The TextBox styles 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!

@nicolaihenriksen nicolaihenriksen added this to the 4.9.0 milestone May 1, 2023
@nicolaihenriksen nicolaihenriksen self-assigned this May 1, 2023
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
@nicolaihenriksen nicolaihenriksen changed the title WIP: Additional SmartHint refactoring Additional SmartHint refactoring May 2, 2023
@nicolaihenriksen nicolaihenriksen marked this pull request as ready for review May 2, 2023 13:25
@nicolaihenriksen nicolaihenriksen requested a review from Keboo May 2, 2023 13:25
@Keboo Keboo mentioned this pull request May 9, 2023
29 tasks
Copy link
Member

@Keboo Keboo left a comment

Choose a reason for hiding this comment

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

This is awesome work! thank you so much!

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" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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 Keboo merged commit 53ceabd into master May 11, 2023
@Keboo Keboo deleted the fix3174 branch May 11, 2023 03:03
@Keboo Keboo added the release notes Items are likely to be highlighted in the release notes. label May 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes Items are likely to be highlighted in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filled Text Box hint position
2 participants