-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
BarBackground with Brush in NavigationPage on theme change #24429
Conversation
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
/rebase |
043a472
to
456816d
Compare
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
...rols/tests/TestCases.Android.Tests/snapshots/android/NavigationBarBackgroundShouldChange.png
Outdated
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
@kubaflo Could you rebase and fix the conflict? Thanks in advance. |
Azure Pipelines successfully started running 3 pipeline(s). |
Azure Pipelines successfully started running 3 pipeline(s). |
/rebase |
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.
Copilot reviewed 6 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (1)
- src/Controls/tests/TestCases.HostApp/Issues/Issue24428.xaml: Language not supported
Comments suppressed due to low confidence (2)
src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs:39
- [nitpick] The variable name _currentBarBackgroundBrush is clear and consistent with existing naming conventions.
Brush _currentBarBackgroundBrush;
src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs:268
- Ensure that the new behavior introduced in UpdateBarBackground and RefreshBarBackground is covered by tests.
if(_currentBarBackgroundBrush is GradientBrush gb)
ceb0ade
to
b511f6e
Compare
/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.
Looking good, I think we can add a verify for the initial light mode just in case something breaks and CI suddenly goes dark mode by default. #defensivetesting
/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.
NavigationBarBackgroundShouldChange is failing on android
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.
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- src/Controls/tests/TestCases.HostApp/Issues/Issue24428.xaml: Language not supported
Comments suppressed due to low confidence (2)
src/Controls/src/Core/Toolbar/Toolbar.Android.cs:108
- Ensure that the new UpdateBarBackground method is covered by tests in TestCases.Shared.Tests.
void UpdateBarBackground()
src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs:723
- The new behavior introduced in OnBarBackgroundChanged is not covered by tests. Ensure that this method is tested to verify the functionality.
void OnBarBackgroundChanged(object sender, EventArgs e)
src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs
Show resolved
Hide resolved
src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs
Show resolved
Hide resolved
/azp run |
Azure Pipelines successfully started running 3 pipeline(s). |
Issues Fixed
Fixes #24428
Screen.Recording.2024-08-26.at.01.04.52.mov