-
Notifications
You must be signed in to change notification settings - Fork 671
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
TextAlignment property added to NumberBox #2901
TextAlignment property added to NumberBox #2901
Conversation
dev/NumberBox/NumberBox.xaml
Outdated
@@ -1,14 +1,13 @@ | |||
<!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT License. See LICENSE in the project root for license information. --> | |||
<ResourceDictionary |
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.
Almost all of those changes are just formatting. Can you please revert the formatting changes?
With all these changes it is hard to review the actual new feature in question.
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.
I just reverted the changes. I had XAML Styler running which moved things around. Sorry.
dev/NumberBox/NumberBox.idl
Outdated
@@ -60,6 +60,7 @@ unsealed runtimeclass NumberBox : Windows.UI.Xaml.Controls.Control | |||
String PlaceholderText; | |||
Windows.UI.Xaml.Controls.Primitives.FlyoutBase SelectionFlyout; | |||
Windows.UI.Xaml.Media.SolidColorBrush SelectionHighlightColor; | |||
Windows.UI.Xaml.TextAlignment TextAlignment; |
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.
Posting previous comment here as this is more suitable in the IDL file.
I think this would technically go through API review first. @MikeHillberg Should this be marked as preview until then?
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.
Yes, this should be reviewed and marked as preview because of an apparent mismatch between the documentation and the API behavior regarding the TextBox.HorizontalTextAlignment and TextBox.TextAlignment APIs.
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.
@sonnemaf until this has an approved spec, you'll need to mark the API as preview, examples can been seen in some other IDL files in the repo
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.
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.
Done!
You can add a test NumberBox which uses the TextAlignment property on the test page inside the NumberBox_TestUI project(which is under /dev/NumberBox), to verify that the changes work correctly. @sonnemaf What kind of information would have helped you on the source code structure documentation to make it easier understanding the source code? |
…s now Left. Also added the TextAlignment properties in the Test app.
The source code is quite clear (although I have no real experience in C++). I just couldn't find the test app. I found the MUXControlsTestApp app and I can now test the app. |
For the API review: @MikeHillberg and I are currently looking at an apparent mismatch between the documentation and the API behavior regarding the |
A spec for this is a good place to have a discussion about those two alignment properties. Also the InputScope property. |
@SavoySchuler FYI |
Value="456" /> | ||
|
||
|
||
<controls:NumberBox x:Name="TestNumberBox" |
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.
Can you please add a Combo box which has the text alignment options as combo box items and selecting one of those will change the TestNumberBox's TextAlignment to the chosen value.
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.
@@ -60,7 +60,12 @@ unsealed runtimeclass NumberBox : Windows.UI.Xaml.Controls.Control | |||
String PlaceholderText; | |||
Windows.UI.Xaml.Controls.Primitives.FlyoutBase SelectionFlyout; | |||
Windows.UI.Xaml.Media.SolidColorBrush SelectionHighlightColor; | |||
Windows.UI.Xaml.TextAlignment TextAlignment; | |||
|
|||
[WUXC_VERSION_PREVIEW] |
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.
You also need to include static Windows.UI.Xaml.DependencyProperty TextAlignmentProperty{ get; };
here.
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.
Done, makes sense.
@@ -226,7 +226,7 @@ void NumberBoxProperties::EnsureProperties() | |||
winrt::name_of<winrt::TextAlignment>(), | |||
winrt::name_of<winrt::NumberBox>(), | |||
false /* isAttached */, | |||
ValueHelper<winrt::TextAlignment>::BoxValueIfNecessary(winrt::TextAlignment::Left), |
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.
Why did you remove the default value? TextBox.TextAlignment/HorizontalTextAlignment uses TextAlignment::Left as their default value and we should pick the same default here.
To add a default value, you add the default value in the NumberBox .idl file by adding [MUX_DEFAULT_VALUE("winrt::TextAlignment::Left")]
above the TextAlignment entry (you can find examples in the .idl file).
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.
Agree with @Felix-Dev here, please set the default value to TextAlignment::Left
.
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.
Don't know what happened with the Default. It is now back to Left. Sorry...
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.
@sonnemaf It appears to me you have manually edited the NumberBox.properties.cpp file to include the default value in commit dc9cef7. Since that file is auto-generated (for example whenever a new API is added to the NumberBox.idl file) manually added code will not persist here. In fact, I wouldn't be surprised if that was the reason the default value was removed in a previous commit (the file was probably automatically generated again and you committed that new version).
Instead, to specify a persisting default value for a public API, please add it to the .idl file like so:
[WUXC_VERSION_PREVIEW]
{
[MUX_DEFAULT_VALUE("winrt::TextAlignment::Left")]
Windows.UI.Xaml.TextAlignment TextAlignment;
}
This will generate the correct code whenever a new version of that file is automatically created. If you build WinUI after adding this code to the NumberBox.idl file and check the generated NumberBox.properties.cpp file, you will see the default value included as desired.
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.
Yes I was editing the NumberBox.properties.cpp manually. I fixed it now using the MUX_DEFAULT_VALUE attribute in the .idl file. C++ is totally new for me. Thanks for your help.
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006" | ||
xmlns:contract5Present="http://schemas.microsoft.com/winfx/2006/xaml/presentation?IsApiContractPresent(Windows.Foundation.UniversalApiContract,5)" | ||
mc:Ignorable="d"> | ||
<!-- Copyright (c) Microsoft Corporation. All rights reserved. Licensed under the MIT License. See LICENSE in the project root for license information. --> |
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.
A lot of these changes are only formatting, please revert them.
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.
Sorry, XAML Styler was interfering again. Disabled it for now.
I have reverted the changes. I should be ok again.
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.
I think you forgot to push that commit 😅
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.
I'd also prefer the styling update to be a separate commit
In reply to: 456551014 [](ancestors = 456551014)
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.
I forgot the Push 😒 and made now two separate commits 😊.
… marked as Preview
… TextAlignment default value using an attribute in the idl.
…nted the TextAlignment default value using an attribute in the idl." This reverts commit 2410590.
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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.
@ranjeshj and @SavoySchuler I think the change looks good assuming the API does. |
Should we also add an API test here? (based on the current implementation: verifying that setting this API sets the corresponding API on the TextBox - for which I assume tests exist to verify its correctness.) |
I think its fine to merge as a preview API. |
I think this is a valid consideration. @sonnemaf do you have time to add a test? |
@sonnemaf Do you need help writing a tests? |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
The NumberBox was missing a TextAlignment property. Now it has one.
Description
I added the TextAlignment dependency property to the NumberBox. It needs testing and documentation. Don't know how to do this. Sorry.
Motivation and Context
Closes #1722
How Has This Been Tested?
Added two NumberBoxes to the test page with Center and Right TextAlignment.