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

- [Android] Fix ShouldShowToolbarButton for FlyoutPage #18522

Merged
merged 1 commit into from Mar 5, 2024

Conversation

bradencohen
Copy link
Contributor

@bradencohen bradencohen commented Nov 3, 2023

Description of Change

Fixes an issue where overriding ShouldShowToolbarButton() in a FlyoutPage does not hide the hamburger icon (drawer toggle) on Android.

In the NavigationPageToolbar, the drawer toggle is marked visible if the parent is a FlyoutPage and there are no pages pushed on to the stack (here).

There should also be a call made to ShouldShowToolbarButton in cases where you want to explicitly hide the toolbar button, which is the hamburger icon.

This has no effect on the back button, which is handled separately.

On iOS, ShouldShowToolbarButton is called in the compatibility NavigationRenderer, which is still used as the primary handler today and explains why it's only broken in Android.

Issues Fixed

Fixes #15111


Hope this helps! Let me know if you need further adjustments.

@bradencohen bradencohen requested a review from a team as a code owner November 3, 2023 22:39
@ghost ghost added the community ✨ Community Contribution label Nov 3, 2023
@ghost
Copy link

ghost commented Nov 3, 2023

Hey there @bradencohen! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@Eilon Eilon added the area/FlyoutPage FlyoutPage label Nov 9, 2023
@bradencohen
Copy link
Contributor Author

Any idea on when this could be reviewed @mattleibow @jsuarezruiz ?

No rush, there is a decent workaround posted, just wanted to see if there was anything else required on my end.

@samhouts samhouts added the stale Indicates a stale issue/pr and will be closed soon label Jan 8, 2024
@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@bradencohen
Copy link
Contributor Author

I see that some checks were unsuccessful. Let me know if there's anything I can do to help resolve that!

@samhouts samhouts removed the stale Indicates a stale issue/pr and will be closed soon label Feb 6, 2024
@rmarinho
Copy link
Member

/rebase

@rmarinho
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz jsuarezruiz self-requested a review February 21, 2024 09:27
@jsuarezruiz
Copy link
Contributor

I want to include some tests in this PR. The same where reproduce the issue is this one https://github.com/GODston/Public right?

@bradencohen
Copy link
Contributor Author

bradencohen commented Feb 27, 2024

I want to include some tests in this PR. The same where reproduce the issue is this one https://github.com/GODston/Public right?

Yes it is. I'm still learning and would be super happy to write some tests, are there any good examples to follow?

I can take a stab at it this weekend 😄. @jsuarezruiz

@rmarinho rmarinho merged commit 9bb4cd1 into dotnet:main Mar 5, 2024
44 of 47 checks passed
@rmarinho
Copy link
Member

rmarinho commented Mar 5, 2024

@jsuarezruiz @bradencohen created a issue to follow up adding a global test for this on all platforms.

#21012

@bradencohen bradencohen deleted the fix-15111 branch March 5, 2024 15:14
@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ShouldShowToolbarButton() override on FlyoutPage not working
5 participants