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.

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

fix(iOS) CommandBar updates on navigation #3017 #4287

Merged
merged 4 commits into from
Oct 22, 2020

Conversation

ajpinedam
Copy link
Contributor

GitHub Issue (If applicable): #
closes #3017

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Changing the Background property on the CommandBar on Page1 while navigating to Page2 changes the CommandBar on Page2.

What is the new behavior?

Changes on one Page CommandBar will not affect other page's CommandBar

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Internal Issue (If applicable):

@ajpinedam ajpinedam requested review from jeromelaban, dr1rrb and a team October 16, 2020 13:34
@ajpinedam ajpinedam self-assigned this Oct 16, 2020
@gitpod-io
Copy link

gitpod-io bot commented Oct 16, 2020

@carldebilly
Copy link
Member

Is there a way to create an automated test for that?

@ajpinedam
Copy link
Contributor Author

Is there a way to create an automated test for that?

Good question but honestly I am not really sure if this is possible. The test would be time-sensitive in order to reproduce it.

@ajpinedam
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ajpinedam ajpinedam force-pushed the dev.anpi/ios.commandbar.color branch from 9c01a8e to 48d1b0a Compare October 17, 2020 00:03
@jeromelaban
Copy link
Member

For the tests, does it reproduce inside an uno sample control, manually ?

@ajpinedam
Copy link
Contributor Author

For the tests, does it reproduce inside an uno sample control, manually ?

To reproduce it we would require 2 pages both with CommandBar and do a navigation between the pages

@davidjohnoliver
Copy link
Contributor

davidjohnoliver commented Oct 19, 2020

For the tests, does it reproduce inside an uno sample control, manually ?

To reproduce it we would require 2 pages both with CommandBar and do a navigation between the pages

Here's a test that's doing that: https://github.com/unoplatform/uno/blob/master/src/SamplesApp/SamplesApp.UITests/Windows_UI_Xaml_Controls/CommandBarTests/UnoSamples_Tests.NativeCommandBar.cs#L68

GitHub
Build Mobile, Desktop and WebAssembly apps with C# and XAML. Today. Open source and professionally supported. - unoplatform/uno

@ajpinedam ajpinedam force-pushed the dev.anpi/ios.commandbar.color branch from d51eeba to 4897a14 Compare October 22, 2020 07:14
@ajpinedam ajpinedam force-pushed the dev.anpi/ios.commandbar.color branch from c03ea51 to f97545a Compare October 22, 2020 07:51
if (pageController.FindTopCommandBar() is { } topCommandBar)
{
// Set the native navigation bar to null so it does not render when the page is not visible
SetNavigationBar(topCommandBar, null!);
Copy link
Contributor

Choose a reason for hiding this comment

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

null! is not a thing you should ever write if you can avoid it. Rather you should change the called signature to SetNavigationItem(CommandBar commandBar, UIKit.UINavigationItem? navigationItem)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

null! is not a thing you should ever write if you can avoid it. Rather you should change the called signature to SetNavigationItem(CommandBar commandBar, UIKit.UINavigationItem? navigationItem)

I just followed the implementation of similar methods ex: PageDidDisappear as I did not want to do refactoring on code I did not add.

@davidjohnoliver

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will push another PR doing this refactoring

@ajpinedam ajpinedam merged commit 5c53c2f into unoplatform:master Oct 22, 2020
@ajpinedam ajpinedam deleted the dev.anpi/ios.commandbar.color branch October 22, 2020 22:05
@jeromelaban
Copy link
Member

@Mergifyio backport release/beta/3.1

@mergify
Copy link
Contributor

mergify bot commented Oct 26, 2020

Command backport release/beta/3.1: success

Backports have been created

jeromelaban added a commit that referenced this pull request Oct 27, 2020
…/pr-4287

fix(iOS) CommandBar updates on navigation #3017 (bp #4287)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changing a CommandBar changes another CommandBar on iOS
4 participants