-
Notifications
You must be signed in to change notification settings - Fork 1.9k
[iOS] Added support for large titles in Shell #32081
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 introduces web resource request interception capabilities to the BlazorWebView component and includes extensive localization updates for .NET MAUI template strings. The primary purpose is to enable applications to intercept and customize web resource requests within BlazorWebView controls, while also updating localized template descriptions across multiple languages.
- Adds web resource request interception functionality to BlazorWebView
- Updates BlazorWebView to implement IWebRequestInterceptingWebView interface
- Includes comprehensive localization updates for template strings in multiple languages
Reviewed Changes
Copilot reviewed 293 out of 3888 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BlazorWebView/src/Maui/IBlazorWebView.cs | Extended interface to inherit from IWebRequestInterceptingWebView |
| src/BlazorWebView/src/Maui/BlazorWebView.cs | Added WebResourceRequested event and implementation |
| src/BlazorWebView/src/Maui/Android/WebKitWebViewClient.cs | Enhanced request interception logic with improved logging |
| src/BlazorWebView/src/Maui/Android/BlazorWebViewHandler.Android.cs | Updated Android WebView integration with namespace changes |
| src/BlazorWebView/src/Maui/PublicAPI/*.txt | Added new public API surface for WebResourceRequested event |
| loc//src/Templates/src/templates/ | Updated localized template strings across multiple languages |
src/BlazorWebView/src/Maui/Microsoft.AspNetCore.Components.WebView.Maui.csproj
Show resolved
Hide resolved
Introduces the UpdateLargeTitles method to manage large title display modes for navigation bars in ShellItemRenderer. This ensures that the large title preference is updated when the displayed page changes or when the view lays out subviews, aligning with iOS-specific platform configurations.
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellItemRenderer.cs
Show resolved
Hide resolved
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
/rebase |
a285a15 to
4db82e9
Compare
|
/rebase |
Co-authored-by: kubaflo <[email protected]>
4db82e9 to
01f1d3d
Compare
|
/azp run MAUI-UITests-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Co-authored-by: kubaflo <[email protected]>
# PR Review: #32081 - [iOS] Added support for large titles in ShellPR: #32081 SummaryPR #32081 implements iOS large navigation bar titles in Shell by adding Status: ✅ Excellent implementation with minor cosmetic cleanup suggested Core implementation: ✅ Solid - proper null checks, iOS version guard, correct enum mapping, appropriate lifecycle hooks Code Review✅ Positive Aspects
🟡 Minor Issues1. Redundant Conditional CompilationFile: Issue: #if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS
How it works: The test project system defines constants for platforms where tests SHOULD NOT run:
So this condition:
Fix (optional cleanup): #if TEST_FAILS_ON_ANDROID && TEST_FAILS_ON_WINDOWS // Remove duplicateImpact: LOW - Code compiles and runs correctly, just has a cosmetic redundancy 🟡 Suggestions for Improvement1. Missing Dynamic Property Change HandlingCurrent behavior:
Gap: If a user changes Comparison: void OnDisplayedPagePropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == Shell.TabBarIsVisibleProperty.PropertyName)
UpdateTabBarHidden();
// Missing: Check for LargeTitleDisplay property changes
}Suggested improvement: void OnDisplayedPagePropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == Shell.TabBarIsVisibleProperty.PropertyName)
UpdateTabBarHidden();
else if (e.PropertyName == PlatformConfiguration.iOSSpecific.Page.LargeTitleDisplayProperty.PropertyName)
UpdateLargeTitles();
}Impact: MEDIUM - Users might want to toggle this dynamically (though uncommon) 2. Performance Consideration - ViewWillLayoutSubviewsObservation: Analysis:
Potential optimization (optional): LargeTitleDisplayMode? _lastLargeTitleDisplayMode;
void UpdateLargeTitles()
{
var page = _displayedPage;
if (page is null || !OperatingSystem.IsIOSVersionAtLeast(11))
return;
var largeTitleDisplayMode = page.OnThisPlatform().LargeTitleDisplay();
// Only update if changed
if (_lastLargeTitleDisplayMode == largeTitleDisplayMode)
return;
_lastLargeTitleDisplayMode = largeTitleDisplayMode;
// ... rest of implementation
}Impact: LOW - Likely not a real performance issue, iOS handles repeated property sets efficiently 3. Test Coverage GapsCurrent test:
Missing scenarios:
Suggested additional tests (future enhancement): [Test]
public void LargeTitleDisplayNever()
{
// Verify standard title bar height
}
[Test]
public void LargeTitleDisplayAutomatic()
{
// Verify automatic behavior (large on first page, small after scroll/navigation)
}Impact: MEDIUM - Current test validates basic functionality, but edge cases untested 🛑 Testing Status - CHECKPOINT REQUIREDEnvironment LimitationIssue: PR review is being performed in a Linux environment without iOS simulators.
Checkpoint for iOS TestingPlatform: iOS 17+ (iPhone Xs or later recommended) Testing steps needed:
Resume instructions: After iOS testing is complete, update this review document with:
Current Recommendation✅ Approve with Optional Improvements Optional Cleanup (Non-Blocking)
Recommended Improvements (Non-Blocking)
Post-iOS-Testing UpdateiOS testing status: Unable to complete due to Linux environment limitation (no iOS simulators available) Recommendation without iOS testing: Based on code review alone, the implementation appears correct:
Ideal validation (when iOS environment is available):
Current recommendation: ✅ Approve - Code quality is excellent, iOS testing would provide additional confidence but is not blocking Related DocumentationReview History
|
|
@kubaflo I think this needs snapshot update? |
Corrects the logic for setting PrefersLargeTitles to only enable large titles when LargeTitleDisplayMode is Always. Updates Issue12156 test to set LargeTitleDisplay on ContentPage instead of Shell. Adds snapshot images for large title display verification on Mac and iOS.
Fixed |
|
/azp run MAUI-UITests-public |
|
Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command. |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run MAUI-public |
|
Azure Pipelines successfully started running 1 pipeline(s). |
Introduces the UpdateLargeTitles method to manage large title display modes for navigation bars in ShellItemRenderer. This ensures that the large title preference is updated when the displayed page changes or when the view lays out subviews, aligning with iOS-specific platform configurations. Added a UITest Fix large title display mode handling on iOS Shell Corrects the logic for setting PrefersLargeTitles to only enable large titles when LargeTitleDisplayMode is Always. Updates Issue12156 test to set LargeTitleDisplay on ContentPage instead of Shell. Adds snapshot images for large title display verification on Mac and iOS.
Introduces the UpdateLargeTitles method to manage large title display modes for navigation bars in ShellItemRenderer. This ensures that the large title preference is updated when the displayed page changes or when the view lays out subviews, aligning with iOS-specific platform configurations. Added a UITest Fix large title display mode handling on iOS Shell Corrects the logic for setting PrefersLargeTitles to only enable large titles when LargeTitleDisplayMode is Always. Updates Issue12156 test to set LargeTitleDisplay on ContentPage instead of Shell. Adds snapshot images for large title display verification on Mac and iOS.
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Description of Change
Introduces the UpdateLargeTitles method to manage large title display modes for navigation bars in ShellItemRenderer. This ensures that the large title preference is updated when the displayed page changes or when the view lays out subviews, aligning with iOS-specific platform configurations.
Issues Fixed
Fixes #12156