-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Refactor SmartHint alignment #3056
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
Refactor SmartHint alignment #3056
Conversation
|
Yea I can see what you mean. I think we may want to revisit it to also handle the right alignment case (since I agree the fixed x position seems wrong). I suspect we may want to setup the Smart hint to better respect the I am fine kicking out another release with this. Though, I would want to pick the current stuff that is in the 4.8.0 milestone. It is possible to cut a revert from the 4.7.2 tag, but I am thinking that it might be nicer to just cut 4.8.0 with the revert. Thoughts? |
I am fine with either. I was just thinking there isn't really anything that has been added (nor planned) for 4.8? There are of course some dependency version bumps... I was just thinking we should revert so people experiencing issues with this don't have to use 4.6, but can use a "latest" release instead. Regarding how to fix the issue: I agree it should respect the HorizontalContentAlignment and float accordingly. For that to work it needs some information... I was thinking about leveraging the IHintProxy and adding a bit more information on there which the SmartHint could use to pull info from its "container". That does however mean that all SmartHint consumers now need to deliver that information (breaking change). I have actually not looked into whether this proxy is mandatory or not, but at least for the floating hint it then would be mandatory. But as I stated in the PR, for this to work properly, I believe the SmartHint needs to know about the placement/alignment as well as the available space it has to work within... |
818db74 to
93f5b40
Compare
|
@Keboo I had another stab at this See the updated PR description for details of the changes Generally I think it works pretty well, and now with a lot more flexibility than before. |
Alignment now respects the HorizontalContentAlignment of the "owner"/"proxy". By default, the hint will float to the same alignment (left/center/right) as the HorizontalContentAlignment. Because I suspect there may be cases a user would want to float the hint differently that the resting position, I added an attached property HintAssist.FloatingHintHorizontalAlignment (inherited in the visual tree) which can be used to override the hint position when the hint is floated. By default, this property returns "Inherit" which means it follows the HorizontalContentAlignment, but it can also be set to Left/Center/Right/Stretch allowing explicit override of the default floating position.
93f5b40 to
029884a
Compare
Keboo
left a comment
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.
This looks like great work!
MaterialDesignThemes.Wpf/Converters/FloatingHintTextBlockMarginConverter.cs
Outdated
Show resolved
Hide resolved
…nConverter.cs Co-authored-by: Kevin B <[email protected]>
What about the new page in the demo app? Is it too exhaustive? Or are you OK with adding it like this? And which branch do we merge it into? Technically it can be considered a breaking change... |
Yea I am good with adding that to the demo app. I have been thinking about trying to create a page in the demo app that can be a bit more exhaustive showing all relevant properties for each of the styles. I say merge it to master. I think we will be having a 4.8.0 release and this can go out with that. |
|
I raised an issue with TextBox's that are multiple lines - this change did impact it. I dont see any tests for multi-line TextBoxes above. Can you add that to your sample? It has an impact on how it renders TextBoxes with a Height configured. Raised it in #3161 |
Fixes #3055
As mentioned in #3055 my recent PR (#3041) caused a regression for input elements with
HorizontalContentAlignment=Right. I had not considered this scenario when coding the fix.This PR refactors the
SmartHintalignment quite extensively, and could potentially be considered a breaking change (I added the label; remove it if you disagree).Essentials of the PR:
Styleof the controls that currently use theSmartHintsuch that theSmartHintis "stretched" to fill the available space, thus allowing it to infer the location of the hint text.SmartHintto also provide theHorizontalContentAlignmentallowing the hint to float accordingly.SmartHintimplementation to use a converter to handle the horizontal alignment of the hint text.HintAssist.FloatingHintHorizontalAlignmentattached property which allows the user to have a different floating/resting alignments which are not the same. I think this may be useful in a "user form" scenario where you stack elements and would like the floating hints to align, but may want to enter values withHorizontalContentAlignment=Rightfor example for currencies.Ideally I would like to eventually write some XAMLTest UI tests that assert these floating alignments are respected, I think I would like your input on how to best go about doing that.
There are still a few minor alignment issues which can be spotted, but most of these are also present in the previous implementations.
Preview of the page in the demo app:
