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.

Sign up for GitHub

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

Jump to bottom

PersonPicture update: Overwriteable properties #6655

Merged
merged 9 commits into from
Aug 21, 2023
Merged

PersonPicture update: Overwriteable properties #6655

merged 9 commits into from
Aug 21, 2023

Conversation

jp-weber
Copy link
Contributor

Description

Make properties of PersonPicture overwriteable. Solves #6654

Motivation and Context

How Has This Been Tested?

Only in debug mode. No UnitTest.

Screenshots (if appropriate):

grafik

@jp-weber
Merge pull request #1 from microsoft/main
99fd72d 
Merge
@jp-weber
Using properties
679603b 
Properties can now be overwritten (Background, BorderBrush and BorderThickness)
@jp-weber
Using properties
225d658 
Properties can now be overwritten (Background, BorderBrush and BorderThickness)
@ghost ghost added the needs-triage Issue needs to be triaged by the area owners label Jan 30, 2022
@ghost
Copy link

ghost commented Jan 30, 2022 •
edited by ghost
Loading

CLA assistant check
All CLA requirements met.

@jp-weber jp-weber mentioned this pull request Jan 30, 2022
Closed
@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters
Copy link
Contributor

@jp-weber the failing VisualVerification tests show the person picture properties changing. I thought this change would have no visual impact by default. Can you explain the diff?

@jp-weber
Update PersonPicture.xaml
a4c3fb9 
fixed wrong BorderBrush
@jp-weber
Copy link
Contributor Author

@jp-weber the failing VisualVerification tests show the person picture properties changing. I thought this change would have no visual impact by default. Can you explain the diff?

@StephenLPeters I see the problem, I have overwritten the BorderBrush in PersonPicture.xaml with the value from PersonPicture_v1.xaml. Now it is correct.

@StephenLPeters
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@StephenLPeters StephenLPeters added area-PersonPicture team-Controls Issue for the Controls team and removed needs-triage Issue needs to be triaged by the area owners labels Mar 4, 2022
@jp-weber jp-weber mentioned this pull request Jan 4, 2023
Closed
@bkudiess
Copy link
Contributor

bkudiess commented Jan 6, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jp-weber
Copy link
Contributor Author

jp-weber commented Jan 8, 2023

@bkudiess the UnitTest VerifyPersonPictureVisualTree has failed. I have no idea what the reason is. ##[error]ApiTests.PersonPictureTests.VerifyPersonPictureVisualTree:
https://github.com/microsoft/microsoft-ui-xaml/blob/main/dev/PersonPicture/APITests/PersonPictureTests.cs#L195

@jp-weber
Copy link
Contributor Author

Push
@bkudiess
What is the problem here that the test fails?

@MartinZikmund
Copy link
Contributor

MartinZikmund commented Apr 24, 2023 •
edited
Loading

@StephenLPeters @bkudiess Any chance this could be merged soon?

@ranjeshj
Copy link
Contributor

ranjeshj commented Aug 7, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Aug 8, 2023 •
edited
Loading

@jp-weber Looks like you need a VisualVerification file update. I pushed the update you need in this commit: 7bf53e9
If you cherry-pick the change this PR should get unblocked.

@jp-weber
Copy link
Contributor Author

jp-weber commented Aug 9, 2023

Thank you @ojhad, I have created a cherry pick from your commit.

@ojhad
Copy link
Contributor

ojhad commented Aug 9, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Aug 10, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ojhad
Copy link
Contributor

ojhad commented Aug 18, 2023

@jp-weber Looks like the script didn't update the visual verification files properly and that test is still failing. Can you delete the test/MUXControlsTestApp/verification/PersonPicture-8.xml file?
I ran this scenario in the pipeline and verified that it passes.

@ojhad
Copy link
Contributor

ojhad commented Aug 21, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@jp-weber
Copy link
Contributor Author

@ojhad
Thanks for creating the commit 👍, unfortunately I didn't have the time to do it these days

@ojhad ojhad merged commit 8efd67e into microsoft:main Aug 21, 2023
32 checks passed
@Youssef1313 Youssef1313 mentioned this pull request Aug 21, 2023
Merged
6 tasks
@jp-weber jp-weber deleted the PersonPicture-update branch August 21, 2023 20:39
@Youssef1313 Youssef1313 mentioned this pull request Nov 1, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ojhad ojhad ojhad approved these changes

@StephenLPeters StephenLPeters StephenLPeters approved these changes

@raghavthind2005 raghavthind2005 raghavthind2005 approved these changes

Assignees
No one assigned
Labels
area-PersonPicture team-Controls Issue for the Controls team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@jp-weber @StephenLPeters @bkudiess @MartinZikmund @ranjeshj @ojhad @raghavthind2005