Skip to content

Conversation

@nicolaihenriksen
Copy link
Contributor

@nicolaihenriksen nicolaihenriksen commented Apr 8, 2023

Fixes #3161

Apparently I had missed this when doing the refactoring, and I must admit I cannot recall exactly why that VerticalAlignment was introduced in the TextBox style. My guess is one of two things happened:

  • Either it is a left over change I forgot to revert when trying various approaches to get the SmartHint to align correctly, or
  • An attempt to align across the PasswordBox and TextBox styles; because The PasswordBox had this alignment already in its 2 styles.

Anyways, this PR removes it as it does not seem needed. I think there may still be an issue though (should probably be created as a separate issue): I would assume that the hint as well as the "text box content" would respect the TextBox.VerticalContentAlignment which I don't think it currently does.

Another thing is, as mentioned in #3161, we should put some UI tests in place to catch regressions in these horizontal/vertical alignment scenarios.

I managed to find a little free time after all. I updated the PR to consider TextBox.VerticalContentAlignment as well, and added UI tests for all VerticalAlignment enum values. For the "non-top-aligned" scenarios, the user will need to manually adjust the HintAssist.FloatingOffset to get correct floating placement of the hint (I have done this for the GIFs below).

@Keboo The UI test is probably not the best test in the world, as it simply asserts the VerticalAlignment value is correctly propagated down to the hint. I guess ideally it should also assert that the absolute position of the hint is "correct" (i.e. top/center/bottom of the TextBox).

VerticalAlignment.Top or VerticalAlignment.Stretch
AlignmentTopOrStretch

VerticalAlignment.Center
AlignmentMiddle

VerticalAlignment.Bottom
AlignmentBottom

@nicolaihenriksen nicolaihenriksen marked this pull request as draft April 8, 2023 12:21
@Keboo Keboo self-requested a review April 8, 2023 16:01
@Keboo Keboo added this to the 4.9.0 milestone Apr 8, 2023
@TWhidden
Copy link
Contributor

TWhidden commented Apr 8, 2023

I had not considered the Password Style as having multi-line would be odd, but for sure can confirm that is the change I made on my local source for the TextBox style and did resolve it. I suppose adding some multi-line textboxes to the UI test app would be good. For the meantime however, this PR looks good to me.

@nicolaihenriksen
Copy link
Contributor Author

I agree, PasswordBox style does not need to consider multiline, but nor was the VerticalAlignment needed; so I removed it for consistency.

@nicolaihenriksen nicolaihenriksen changed the title WIP: Removed not needed VerticalAlignment TextBox: Removed not needed VerticalAlignment and it now respects VerticalContentAlignment Apr 8, 2023
@nicolaihenriksen nicolaihenriksen marked this pull request as ready for review April 8, 2023 19:10
PART_ContentHost now uses both VerticalAlignment and VerticalContentAlignment; the test now reflects that as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

TextBox with Height puts HintAssist.Hint right in the middle instead of the top.

4 participants