-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS] Flyout Menu CollectionView First Item Misaligned - fix #30501
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
|
Hey there @@kubaflo! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
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.
Pull Request Overview
This PR fixes the misalignment of the first item in the iOS Shell flyout by removing the legacy SetHeaderContentInset logic and consolidating header offset calculations.
- Remove all calls to
SetHeaderContentInsetand delete the method implementation - Simplify header offset logic in
LayoutContentto always use the header’s frame height - Drop obsolete call in
ViewSafeAreaInsetsDidChange
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellTableViewController.cs | Removed the SetHeaderContentInset call in safe-area changes |
| src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellFlyoutLayoutManager.cs | Deleted SetHeaderContentInset method and calls; simplified the header offset logic |
Comments suppressed due to low confidence (2)
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellFlyoutLayoutManager.cs:191
- This layout change affects the iOS flyout header and first-item alignment; consider adding corresponding UI tests in TestCases.HostApp and TestCases.Shared.Tests to verify correct positioning under both scrollable and non-scrollable scenarios.
internal void UpdateHeaderSize()
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellFlyoutLayoutManager.cs:289
- By removing the conditional branch for ScrollView, the offset now always uses the header height instead of margin for scrollable content. Please verify that this doesn't reintroduce layout issues in scrollable scenarios, or restore a margin-based offset path if needed.
contentYOffset += HeaderView.Frame.Height;
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
5c96df0 to
2fac333
Compare
|
Thanks @kubaflo, this makes sense! |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@kubaflo can you add a test for this? |
|
I've added a test |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
REVERTED
Description of Change
The regression is caused by this PR: #28670, but I think that the changes introduced by @albyrock87 let us simplify the code and easily fix the regression
Issues Fixed
Fixes #30483
Screen.Recording.2025-07-09.at.02.08.10.mov
Screen.Recording.2025-07-09.at.02.07.27.mov