-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS] Fix for Shell custom FlyoutIcon display problem #26016
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
Conversation
/azp run |
This comment was marked as outdated.
This comment was marked as outdated.
@@ -336,7 +336,7 @@ void UpdateLeftToolbarItems() | |||
|
|||
if (image != null) | |||
{ | |||
icon = result?.Value; | |||
icon = ResizeImage(result?.Value, new CGSize(23f, 23f)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
maui/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellPageRendererTracker.cs
Line 495 in c363191
var rect = new CGRect(0, 0, 23f, 23f); |
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
// https://github.com/dotnet/maui/issues/26148 |
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Address some review feedback and suggestions
@@ -219,4 +219,4 @@ body: | |||
- type: markdown | |||
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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