Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BarBackground with Brush in NavigationPage on theme change #24429

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

BarBackground with Brush in NavigationPage on theme change #24429

wants to merge 5 commits into from

Conversation

kubaflo
Copy link
Contributor

@kubaflo kubaflo commented Aug 25, 2024

Issues Fixed

Fixes #24428

Screen.Recording.2024-08-26.at.01.04.52.mov

@kubaflo kubaflo requested a review from a team as a code owner August 25, 2024 23:14
@kubaflo kubaflo requested review from Eilon and tj-devel709 August 25, 2024 23:14
@dotnet-policy-service Dotnet Policy Service
Copy link
Contributor

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.

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Aug 25, 2024
@kubaflo kubaflo added area-navigation NavigationPage platform/iOS 🍎 partner/android Issues for the Android SDK and removed community ✨ Community Contribution labels Aug 25, 2024
@rmarinho
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo added the community ✨ Community Contribution label Sep 2, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jonpryor jonpryor added platform/android 🤖 and removed partner/android Issues for the Android SDK labels Oct 28, 2024
@AlleSchonWeg AlleSchonWeg mentioned this pull request Nov 8, 2024
Closed
@jsuarezruiz
Copy link
Contributor

/rebase

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Nov 15, 2024 •
edited by jfversluis
Loading

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

@kubaflo Could you rebase and fix the conflict? Thanks in advance.

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen dismissed jsuarezruiz’s stale review December 6, 2024 21:34

Snapshots updated

@PureWeen PureWeen added this to the .NET 9 SR3 milestone Dec 6, 2024
@PureWeen PureWeen requested a review from Copilot January 6, 2025 08:31
@PureWeen
Copy link
Member

PureWeen commented Jan 6, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Jan 6, 2025

/rebase

Copilot
Copilot AI reviewed Jan 6, 2025
View reviewed changes

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)
@PureWeen
Copy link
Member

PureWeen commented Jan 6, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

mattleibow
mattleibow reviewed Jan 6, 2025
View reviewed changes
Copy link
Member

@mattleibow mattleibow left a 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

src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue24428.cs Show resolved Hide resolved
mattleibow
mattleibow previously approved these changes Jan 6, 2025
View reviewed changes
@PureWeen
Copy link
Member

PureWeen commented Jan 6, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Jan 7, 2025

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen requested changes Jan 8, 2025
View reviewed changes
Copy link
Member

@PureWeen PureWeen left a 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

@PureWeen PureWeen requested a review from Copilot January 8, 2025 18:06
Copilot
Copilot AI reviewed Jan 8, 2025
View reviewed changes
Copy link

@Copilot Copilot AI left a 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
@@ -36,6 +36,8 @@ public class NavigationRenderer : UINavigationController, INavigationViewHandler
bool _hasNavigationBar;
UIImage _defaultNavBarShadowImage;
UIImage _defaultNavBarBackImage;
Brush _currentBarBackgroundBrush;
Copy link
Preview

Copilot AI Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduction of _currentBarBackgroundBrush in the public API could potentially be a breaking change. Ensure that this change is communicated and documented appropriately.

This comment was generated based on a coding guideline created by a repository admin.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs
@@ -36,6 +36,8 @@ public class NavigationRenderer : UINavigationController, INavigationViewHandler
bool _hasNavigationBar;
UIImage _defaultNavBarShadowImage;
UIImage _defaultNavBarBackImage;
Brush _currentBarBackgroundBrush;
Color _currentBarBackgroundColor;
Copy link
Preview

Copilot AI Jan 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Introduction of _currentBarBackgroundColor in the public API could potentially be a breaking change. Ensure that this change is communicated and documented appropriately.

This comment was generated based on a coding guideline created by a repository admin.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

Copilot Copilot left review comments

@PureWeen PureWeen PureWeen requested changes

@jsuarezruiz jsuarezruiz jsuarezruiz left review comments

@mattleibow mattleibow mattleibow left review comments

@Eilon Eilon Awaiting requested review from Eilon Eilon is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

@tj-devel709 tj-devel709 Awaiting requested review from tj-devel709 tj-devel709 is a code owner automatically assigned from dotnet/dotnet-maui-reviewers

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
area-navigation NavigationPage community ✨ Community Contribution platform/android 🤖 platform/iOS 🍎
Projects
MAUI SDK Ongoing
Status: Changes Requested
Milestone
.NET 9 SR3
Development

Successfully merging this pull request may close these issues.

AppThemeBinding BarBackground with Brush in NavigationPage not working
6 participants
@kubaflo @rmarinho @jsuarezruiz @PureWeen @mattleibow @jonpryor