[iOS] Fix for Shell custom FlyoutIcon display problem#26016
[iOS] Fix for Shell custom FlyoutIcon display problem#26016rmarinho merged 18 commits intodotnet:net10.0from
Conversation
|
/azp run |
This comment was marked as outdated.
This comment was marked as outdated.
| if (image != null) | ||
| { | ||
| icon = result?.Value; | ||
| icon = ResizeImage(result?.Value, new CGSize(23f, 23f)); |
There was a problem hiding this comment.
Is iOS the only one that doesn't resize?
Is this size correct under all the screen options? (@2x etc) https://developer.apple.com/design/human-interface-guidelines/toolbars
There was a problem hiding this comment.
Is iOS the only one that doesn't resize? Is this size correct under all the screen options? (@2x etc) https://developer.apple.com/design/human-interface-guidelines/toolbars
@jsuarezruiz, I have tested the icon under all device screen options, and it works properly on all devices. The size I am using for the custom icon is based on the default Hamburger icon size. Please refer to the code below for your reference
Reference for icon size:
| @@ -0,0 +1,27 @@ | |||
| #if !WINDOWS | |||
| // In Windows, the foreground color is not applied to the custom icon, | |||
| // so as of now, the test is not applicable for Windows. | |||
There was a problem hiding this comment.
Windows is the only one having a different behavior, right?
Could you open a new issue and include the link in the comment.
There was a problem hiding this comment.
Windows is the only one having a different behavior, right? Could you open a new issue and include the link in the comment.
@jsuarezruiz, the new issue for the Windows platform has been logged and I have added a comment. Could you please check and provide your concerns if any?
|
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellPageRendererTracker.cs
Outdated
Show resolved
Hide resolved
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
PureWeen
left a comment
There was a problem hiding this comment.
Can you split the android change out from this PR?
I think the resizing parts of this for the iOS Icon is going to need a little bit deeper of an analysis.
Like, I'm curious if users have an icon that's 30x30, or just slightly bigger than 40x40 if we want to resize.
Playing with the sandbox here's a few sizes
I don't know if we can force an opinion here on the size of that icon for users. If this is an issue in our template we should generate an image at the size it should be
Hi @PureWeen , I have created a separate PR 27502 for Android changes. Please review it and share your concerns. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
| if (maxResizeFactor > 1 && !shouldScaleUp) | ||
| return sourceImage; | ||
|
|
||
| return UIImage.FromImage(sourceImage.CGImage, sourceImage.CurrentScale / maxResizeFactor, sourceImage.Orientation); |
There was a problem hiding this comment.
UIImage.FromImage creates a new instance of UIImage with the specified parameters. Could modify the implementation to just transform the current one?
internal static UIImage ResizeImageSource(this UIImage sourceImage, nfloat maxWidth, nfloat maxHeight, CGSize originalImageSize, bool shouldScaleUp = false)
{
if (sourceImage?.CGImage is null)
return null;
maxWidth = (nfloat)Math.Min(maxWidth, originalImageSize.Width);
maxHeight = (nfloat)Math.Min(maxHeight, originalImageSize.Height);
var sourceSize = sourceImage.Size;
var maxResizeFactor = (nfloat)Math.Min(maxWidth / sourceSize.Width, maxHeight / sourceSize.Height);
if (maxResizeFactor > 1 && !shouldScaleUp)
return sourceImage;
var newSize = new CGSize(sourceSize.Width * maxResizeFactor, sourceSize.Height * maxResizeFactor);
UIGraphics.BeginImageContextWithOptions(newSize, false, 0.0f);
sourceImage.Draw(new CGRect(0, 0, newSize.Width, newSize.Height));
var resizedImage = UIGraphics.GetImageFromCurrentImageContext();
UIGraphics.EndImageContext();
return resizedImage;
}
This approach uses the image context to resize the image, should be more efficient than creating a new instance.
There was a problem hiding this comment.
@jsuarezruiz, previously, I used a similar drawing code without scaling in this PR. As per @mattleibow
suggestion, I utilized the ResizeImageSource method from Button.iOS and have now moved it to the UIImageExtensions class as discussed.
Do I need to modify the existing code as you suggested?
please check the previous discussion and share the details:
#26016 (comment)
There was a problem hiding this comment.
yeah for now we can just keep this as is, but maybe create a issue to in another PR try refactor the code to what @jsuarezruiz says
|
/rebase |
2c23c5f to
b493d9a
Compare
b87ff23 to
abec60b
Compare
6ffe51f to
281213c
Compare
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellPageRendererTracker.cs
Show resolved
Hide resolved
rmarinho
left a comment
There was a problem hiding this comment.
Address some review feedback and suggestions
| attributes: | ||
| value: | | ||
| By opening the issue you agree to follow this project's [Code of Conduct](https://github.com/dotnet/maui/blob/main/.github/CODE_OF_CONDUCT.md) | ||
| By opening the issue you agree to follow this project's [Code of Conduct](https://github.com/dotnet/maui/blob/main/.github/CODE_OF_CONDUCT.md) No newline at end of file |
There was a problem hiding this comment.
its adding spaces instead of tabs
@rmarinho, Even though I copied the file content from the base branch to my branch and there were no actual changes to push, it's showing as a difference only in the PR, even though there are no real changes locally. |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
rmarinho
left a comment
There was a problem hiding this comment.
I saw some issues, but they also reproduce on main, the size/resize of the icon works well this this PR .








Issue: The flyout icon is aligned to the center instead of the left, causing the title to extend beyond the view.
Root Cause of the issue
Description of Change
Reference for icon size: https://github.com/dotnet/maui/blob/main/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellPageRendererTracker.cs#L450
Issues Fixed
Fixes #25920
Tested the behaviour in the following platforms
Screenshot
iOS