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

Add new TextBlock styles #4108

Merged

Conversation

marcelwgn
Copy link
Collaborator

Description

Adding new TextBlock styles and update the TextBox test page to include TextBlock samples of the new style.

Motivation and Context

Closes #4106

How Has This Been Tested?

Tested manually

Screenshots (if appropriate):

Image showing the new textblock styles with their respective name

@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Feb 5, 2021
@mdtauk
Copy link
Contributor

mdtauk commented Feb 5, 2021

YAY 10X style typography has arrived!

Now to cajole the Windows 10 Settings team into updating the inbox app into using them (or just moving to the 10X app ASAP) 😁

</Style>

<Style x:Key="CaptionTextBlockStyle" TargetType="TextBlock" BasedOn="{StaticResource BaseTextBlockStyle}">
<Setter Property="FontSize" Value="12" />
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not hard code 12, please use TextBoxIconFontSize

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having a TextBlock style depend on a TextBox resource sounds a bit confusing to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't notice the difference between block and box.
You can create a new keyed resource

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added new lightweight styling resources for all font sizes.

</Style>

<Style x:Key="BodyTextBlockStyle" TargetType="TextBlock" BasedOn="{StaticResource BaseTextBlockStyle}">
<Setter Property="FontSize" Value="14" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't hard code this value

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Would you like me to create themeresources for all the font sizes defined in the styles or just specific ones?

Copy link
Contributor

Choose a reason for hiding this comment

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

generally speaking. yes. With a keyed Themeresource, user is able to change the size easily without re-template the the style.
The side effect is a little performance penalty.
Of course, these styles are very simple, so I let @YuliKl or @ranjeshj to make the decision.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added new resources now though I think that either way doesn't make a big difference.

dev/CommonStyles/CommonStyles.vcxitems Show resolved Hide resolved
@StephenLPeters StephenLPeters added area-TextBlocks TextBlock, RichTextBlock team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Feb 6, 2021
@ranjeshj ranjeshj requested a review from YuliKl February 8, 2021 18:28
@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Copy link
Contributor

@licanhua licanhua left a comment

Choose a reason for hiding this comment

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

:shipit:

Comment on lines 5 to 11
<x:Double x:Key="CaptionTextBlockFontSize">12</x:Double>
<x:Double x:Key="BodyTextBlockFontSize">14</x:Double>
<x:Double x:Key="BodyStrongTextBlockFontSize">14</x:Double>
<x:Double x:Key="SubtitleTextBlockFontSize">20</x:Double>
<x:Double x:Key="TitleTextBlockFontSize">20</x:Double>
<x:Double x:Key="TitleLargeTextBlockFontSize">40</x:Double>
<x:Double x:Key="DisplayTextBlockFontSize">68</x:Double>
Copy link

Choose a reason for hiding this comment

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

@licanhua is this really necessary? We didn't have the font sizes called out in generic.xaml. Seems like we're adding another layer of obfuscation for ... maybe no good reason?

dev/CommonStyles/TextBlock_themeresources.xaml Outdated Show resolved Hide resolved
Comment on lines 6 to 7
<x:Double x:Key="BodyTextBlockFontSize">14</x:Double>
<x:Double x:Key="BodyStrongTextBlockFontSize">14</x:Double>
Copy link

Choose a reason for hiding this comment

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

Even if we keep these resources, I don't think we need distinct onces for body vs body strong. They're both body size (14), with "strong" referring to font weight. The font size for anything called "body" should stay in sync. So I would simply delete BodyStrongTextBlockFontSize and use BodyTextBlockFontSize inside the body strong style.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed that in the new styles project, left the old styles project as is since we don't want to interfere with existing styles and as such need to either introduce a lightweight styling resource for the BodyStrongTextBlock fontsize or leave them out of that completely.

dev/CommonStyles/TextBlock_themeresources.xaml Outdated Show resolved Hide resolved
@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ranjeshj
Copy link
Contributor

@chingucoding looks like we have a failure in the visual baseline comparision.

@marcelwgn
Copy link
Collaborator Author

@ranjeshj Updated the verification files now and merged with master.

@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@licanhua
Copy link
Contributor

@chingucoding Can you update the master files from the build pipeline, or cherry-pick this commit from user/canhua/text

8bbff51

@marcelwgn
Copy link
Collaborator Author

Updated verification files now @licanhua.

@ranjeshj
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@licanhua
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@marcelwgn
Copy link
Collaborator Author

@kmahone Is there anything you/we could do to make the infra more resilient against this:

Starting: OutputTestResults.ps1
==============================================================================
Task         : PowerShell
Description  : Run a PowerShell script on Linux, macOS, or Windows
Version      : 2.180.1
Author       : Microsoft Corporation
Help         : https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/powershell
==============================================================================
Generating script.
Formatted command: . 'D:\a\1\s\build\Helix\OutputTestResults.ps1' -MinimumExpectedTestsExecutedCount '3000' -CheckJobAttempt $true
========================== Starting Command Output ===========================
"C:\windows\System32\WindowsPowerShell\v1.0\powershell.exe" -NoLogo -NoProfile -NonInteractive -ExecutionPolicy Unrestricted -Command ". 'D:\a\_temp\9c78780d-8968-49a3-8c1b-81f172dd6a5a.ps1'"
Checking test results...
queryUri = https://dev.azure.com/ms/microsoft-ui-xaml/_apis/test/runs?buildUri=vstfs:///Build/Build/140748&includeRunDetails=true&api-version=5.0
Processing results from test run 'windows.10.amd64.clientrs5.open.xaml'
Processing results from test run 'windows.10.amd64.clientrs2.open.xaml'
Skipping test run 'windows.10.amd64.clientrs5.open.xaml', since we have already processed a test run of that name.
Skipping test run 'windows.10.amd64.clientrs2.open.xaml', since we have already processed a test run of that name.
Processing results from test run 'windows.10.amd64.clientrs3.open.xaml'
Invoke-RestMethod : Unable to read data from the transport connection: An existing connection was forcibly closed by 
the remote host.
At D:\a\1\s\build\Helix\OutputTestResults.ps1:53 char:20
+ ... stResults = Invoke-RestMethod -Uri "$($testRun.url)/results?api-versi ...
+                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Invoke-RestMethod], IOException
    + FullyQualifiedErrorId : System.IO.IOException,Microsoft.PowerShell.Commands.InvokeRestMethodCommand
 
##[error]PowerShell exited with code '1'.
Finishing: OutputTestResults.ps1

Or is this just something we have to accept to happen occasionally?

@kmahone
Copy link
Member

kmahone commented Feb 19, 2021

@chingucoding we hit that issue from time to time where the REST call that we make fails due to random server side issues. I was able to work around this by adding some retry logic to the script, but that is in the WinUI3 fork and has not been back-ported to the WinUI2 repo. If you are interested in making the change, I can share the updated script with you.

@marcelwgn
Copy link
Collaborator Author

Sure, if I can help with backporting and it's not too complicated, feel free to share the updated script with me and I'll create a PR for this!

@kmahone
Copy link
Member

kmahone commented Feb 19, 2021

Sure, if I can help with backporting and it's not too complicated, feel free to share the updated script with me and I'll create a PR for this!

I've attached an updated version of AzurePipelinesHelperScripts.ps1, that includes simple implementations of Download-FileWithRetries, Invoke-RestMethodWithRetries and Download-StringWithRetries. Update OutputTestResults.ps1, ProsessHelixFiles.ps1 and UpdateUnreliableTests.ps1 so that they call these functions instead of Invoke-RestMethod, DownloadFile and DownloadString.

The Azure DevOps rest apis are usually pretty stable, so only the calls to the Helix apis need to be updated.

AzurePipelinesHelperScripts.txt

@marcelwgn
Copy link
Collaborator Author

Thanks for sharing @kmahone , I'll try to get this over the weekend and create a PR.

@kmahone
Copy link
Member

kmahone commented Feb 19, 2021

Thanks for sharing @kmahone , I'll try to get this over the weekend and create a PR.

Excellent. Thanks for jumping on this! It should make the test passes more reliable.

@ranjeshj ranjeshj merged commit 3f0ca9f into microsoft:master Feb 19, 2021
marcelwgn added a commit to marcelwgn/microsoft-ui-xaml that referenced this pull request Mar 5, 2021
* Add new TextBlock styles and update test page for textbox to include samples

* Update 2.5 file

* Add lightweight styling resources

* CR feedback

* Update verification files

* Update verification files
Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Oct 30, 2023
Kinnara added a commit to Kinnara/ModernWpf that referenced this pull request Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-TextBlocks TextBlock, RichTextBlock team-Controls Issue for the Controls team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update text block styles
7 participants