Skip to content

Conversation

@nicolaihenriksen
Copy link
Contributor

@nicolaihenriksen nicolaihenriksen commented Jan 20, 2023

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 SmartHint alignment quite extensively, and could potentially be considered a breaking change (I added the label; remove it if you disagree).

Essentials of the PR:

  • Changes the MDIX Style of the controls that currently use the SmartHint such that the SmartHint is "stretched" to fill the available space, thus allowing it to infer the location of the hint text.
  • Expands the usages of the SmartHint to also provide the HorizontalContentAlignment allowing the hint to float accordingly.
  • Changes the SmartHint implementation to use a converter to handle the horizontal alignment of the hint text.
  • Adds a HintAssist.FloatingHintHorizontalAlignment attached 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 with HorizontalContentAlignment=Right for example for currencies.
  • Extends the demo app with a dedicated page which allows extensive manual testing of all the floating hint options. This page may be too exhaustive for a demo application and we may want to trim down to some fewer samples, but it was very useful for manual testing and ensuring this work as expected.

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:
SmartHint

@Keboo Keboo added this to the 4.8.0 milestone Jan 23, 2023
@Keboo
Copy link
Member

Keboo commented Jan 23, 2023

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 HorizontalContentAlignment (allowing it to float to the right when it is set to Right, and left otherwise).

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?

@nicolaihenriksen
Copy link
Contributor Author

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...

@nicolaihenriksen nicolaihenriksen changed the title Demonstrate floating hint issue Refactor SmartHint alignment Jan 27, 2023
@nicolaihenriksen nicolaihenriksen added breaking change Items here have breaking API changes. visual breaking change Items here have affected the visual look of controls. demo app Items that relate to the demo application labels Jan 27, 2023
@nicolaihenriksen
Copy link
Contributor Author

nicolaihenriksen commented Jan 27, 2023

@Keboo I had another stab at this SmartHint alignment, and I think I have it working pretty good now. Give the branch a spin and see what you think. If you spot any issues/concerns (especially regarding the changes in the styles), please let me know so we can test them out and fix any issues.

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.
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 looks like great work!

@nicolaihenriksen
Copy link
Contributor Author

nicolaihenriksen commented Jan 30, 2023

This looks like great work!

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... master or 5.0.0?

@nicolaihenriksen nicolaihenriksen marked this pull request as ready for review January 30, 2023 08:36
@Keboo
Copy link
Member

Keboo commented Jan 30, 2023

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... master or 5.0.0?

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.

@TWhidden
Copy link
Contributor

TWhidden commented Apr 7, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Items here have breaking API changes. demo app Items that relate to the demo application 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.

Floating hint textblock cut off for right-aligned fields

3 participants