Added automation peers for BladeView and BladeItem controls #3518
Conversation
|
Thanks jamesmcroft for opening a Pull Request! The reviewers will test the PR and highlight if there is any conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌 |
ghost
requested review from
ghost
added
the
Rosuavio
left a comment
Rosuavio
left a comment
There was a problem hiding this comment.
The PR is failing CI because it does not meet some of our formatting requirements.
BaldeItem.cs and BladeView.cs. To keep on an eye on these erros, when you build your solution look out for warnings from StyleCop many of them fallowing the patern of SAxxxx.
…b.com/windows-toolkit/WindowsCommunityToolkit into jamesmcroft/3517-bladeview-automation
|
This PR has been marked as "needs attention 👋" and awaiting a response from the team. |
Rosuavio
left a comment
Rosuavio
left a comment
There was a problem hiding this comment.
Looks good now. Thanks, @jamesmcroft!
|
@jamesmcroft similar questions here to the comments on the Carousel one #3507 |
I'll bring the similar changes over to this. |
| /// </returns> | ||
| protected override string GetNameCore() | ||
| { | ||
| int? index = this.OwnerBladeItem.ParentBladeView.GetBladeItems().ToList().IndexOf(this.OwnerBladeItem); |
There was a problem hiding this comment.
You should probably add 1 to the index so the blades will be named "blade 1", "blade 2",.. instead of "blade 0", "blade 1",... This will be more "natural" for the users.
Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeItemAutomationPeer.cs
Outdated
Show resolved
Hide resolved
Microsoft.Toolkit.Uwp.UI.Controls/BladeView/BladeViewAutomationPeer.cs
Outdated
Show resolved
Hide resolved
Rosuavio
dismissed
their stale review
@vgromfeld 's review points out some imporant issues that need addressing.
|
Also unit tests for this one too :) reference sample |
| } | ||
|
|
||
| if (this.OwnerBladeItem != null && !string.IsNullOrEmpty(this.OwnerBladeItem.Name)) | ||
| TextBlock textBlock = this.OwnerBladeItem.FindDescendant<TextBlock>(); |
There was a problem hiding this comment.
@jamesmcroft the BladeItem.Header should be the 'Title' of the Blade, do we want to try and explicitly convert that to a string as a first step?
There was a problem hiding this comment.
Sorted! Apologies for the delay on sorting this out.
|
@jamesmcroft think there's just the small comment I left and then this is good, eh? |
|
@michael-hawker ah yep, this has been on my radar. I'll get this sorted! |
|
@michael-hawker just an update on this, sorted out the comments above. Apologies for not notifying you earlier |
delete-merged-branch
bot
deleted the
jamesmcroft/3517-bladeview-automation
branch
4 participants
Fixes #3517
Changes have been made to introduce an
AutomationPeerfor theBladeViewandBladeItemcontrol to improve the ability for UI testing.PR Type
What kind of change does this PR introduce?
What is the current behavior?
When using a UI Automation tree tool, it is difficult to write automated UI tests for the
BladeViewas the rendered tree doesn't expose the hierarchy of the UI element and children in a sensible manner.What is the new behavior?
A
BladeViewAutomationPeerandBladeItemAutomationPeerhave been added which is used by their respected control implementation in order to surface more automation properties and hierarchy.The change includes minor changes to the
BladeViewandBladeItemcontrols exposed as internal for use by the newAutomationPeerclasses.Below is the before and after of the UI Automation tree.
Before
After
As you can see from the after screenshot, the change surfaces up the
BladeViewand the associated childBladeItemelements within the UI Automation tree.PR Checklist
Please check if your PR fulfills the following requirements: