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

Wrap Image with a container on Windows so that it is centered with AspectFill #17665

Merged
merged 10 commits into from
Aug 21, 2024
Merged

Wrap Image with a container on Windows so that it is centered with AspectFill #17665

merged 10 commits into from
Aug 21, 2024

Conversation

japarson
Copy link
Contributor

@japarson japarson commented Sep 26, 2023 •
edited by samhouts
Loading

Description of Change

On Windows, we use Microsoft.UI.Xaml.Controls.Image which has a stretch property. When this property is set in WinUI, the image will not be centered. According to the WinUI team, this is intended behavior:

image

As you can see in the screenshot above, one workaround we can utilize is to wrap the image in a Grid. Luckily, we can utilize the existing WrapperView on Windows by adding a ContainerView to the Windows ImageHandler.

Before

image

After

image

Issues Fixed

Fixes #10645

Copied from #15122

Depends on

@japarson japarson requested a review from a team as a code owner September 26, 2023 19:04
@Eilon Eilon added the area-image Image loading, sources, caching label Sep 26, 2023
@hartez hartez added this to the .NET 8 + Servicing milestone Sep 26, 2023
emaf
emaf previously approved these changes Oct 6, 2023
View reviewed changes
hartez
hartez suggested changes Oct 6, 2023
View reviewed changes
Copy link
Contributor

@hartez hartez left a comment •
edited
Loading

Choose a reason for hiding this comment

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

We need to resolve our design discussion about how to handle remapping within Core before we can merge this. Scheduled a meeting, we'll update this as soon as we figure it out.

@PureWeen PureWeen mentioned this pull request Oct 9, 2023
Closed
@samhouts samhouts modified the milestones: .NET 8 + Servicing, Under Consideration Oct 9, 2023
@samhouts samhouts added the do-not-merge Don't merge this PR label Oct 9, 2023
@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Nov 21, 2023
@PureWeen
Copy link
Member

/rebase

@jsuarezruiz jsuarezruiz removed their request for review January 24, 2024 11:57
@samhouts samhouts added platform/windows 🪟 and removed stale Indicates a stale issue/pr and will be closed soon labels Feb 1, 2024
@PureWeen
Copy link
Member

For now let's just merge this without the updated extensions.
And we can just fix those in post

@PureWeen PureWeen removed this from the Under Consideration milestone Mar 27, 2024
@Foda
Copy link
Member

Foda commented Mar 28, 2024

/rebase

@Foda
Copy link
Member

Foda commented Mar 28, 2024

@japarson Could you please resolve conflicts and add a UI test? Thanks!

@japarson japarson force-pushed the dev/japarson/10645 branch from cf1b0aa to 4bcd85b Compare April 16, 2024 20:30
@japarson
Copy link
Contributor Author

/rebase

@github-actions github-actions bot force-pushed the dev/japarson/10645 branch from c5068cb to d6f40dd Compare April 19, 2024 18:15
@japarson
Copy link
Contributor Author

@japarson Could you please resolve conflicts and add a UI test? Thanks!

@Foda Test is added and conflicts are resolved.

Foda
Foda previously approved these changes Apr 23, 2024
View reviewed changes
Copy link
Member

@Foda Foda left a comment

Choose a reason for hiding this comment

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

LGTM

@Eilon Eilon added area-controls-image Image control and removed area-image Image loading, sources, caching labels May 10, 2024
jsuarezruiz
jsuarezruiz reviewed May 13, 2024
View reviewed changes
src/Controls/tests/UITests/snapshots/windows/Issue10645Test.png Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

With the changes in the UI Test runner, we need to regenerate the snapshot (now navigates directly to the issue page, and the NavigationBar will no be rendered).

Copy link
Contributor

Choose a reason for hiding this comment

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

Running a new build.

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Merge branch 'main' into dev/japarson/10645
8a122f9 
# Conflicts:
#	src/Controls/tests/TestCases.HostApp/Issues/Issue10645.cs
#	src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue10645.cs
#	src/Controls/tests/TestCases.WinUI.Tests/snapshots/windows/Issue10645Test.png
PureWeen
PureWeen previously approved these changes Aug 16, 2024
View reviewed changes
@PureWeen PureWeen removed the do-not-merge Don't merge this PR label Aug 16, 2024
PureWeen
PureWeen requested changes Aug 17, 2024
View reviewed changes
Copy link
Member

@PureWeen PureWeen left a comment

Choose a reason for hiding this comment

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

screen shot for Issue10645Test is failing

@Foda can you check if we just need to update screen shot? or if something broke?

@PureWeen PureWeen enabled auto-merge (squash) August 20, 2024 18:38
@rmarinho rmarinho disabled auto-merge August 21, 2024 09:26
@rmarinho rmarinho merged commit 57f4c18 into main Aug 21, 2024
95 of 97 checks passed
@rmarinho rmarinho deleted the dev/japarson/10645 branch August 21, 2024 09:26
Redth pushed a commit that referenced this pull request Aug 22, 2024
@japarson @jsuarezruiz
@PureWeen @Redth
Wrap Image with a container on Windows so that it is centered with As…
46735c3 
…pectFill (#17665)

* Wrap Image with a container on Windows so that it is centered with AspectFill

* Add ui test

* Rename test files to correct issue

* Update Issue10645.cs

* Add files via upload

* - fix namespace

* - remove windows

* Update ref image to remove titlebar

---------

Co-authored-by: Javier Suárez <javiersuarezruiz@hotmail.com>
Co-authored-by: Shane Neuville <shneuvil@microsoft.com>
Co-authored-by: Mike Corsaro <mikecorsaro@microsoft.com>
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-net8.0-nightly This may be available in a nightly release! labels Aug 27, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jsuarezruiz jsuarezruiz jsuarezruiz left review comments

@hartez hartez hartez requested changes

@PureWeen PureWeen PureWeen approved these changes

@emaf emaf emaf left review comments

@Foda Foda Foda left review comments

@mattleibow mattleibow Awaiting requested review from mattleibow

Assignees
No one assigned
Labels
area-controls-image Image control fixed-in-net8.0-nightly This may be available in a nightly release! fixed-in-net9.0-nightly This may be available in a nightly release! platform/windows 🪟
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Windows] Image is not centered in AspectFill mode
9 participants
@japarson @PureWeen @Foda @jsuarezruiz @hartez @emaf @Eilon @rmarinho @samhouts