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

Hide FlyoutIcon when FlyoutBehavior set to disabled #20087

Merged
merged 8 commits into from
Jul 17, 2024
Merged

Hide FlyoutIcon when FlyoutBehavior set to disabled #20087

merged 8 commits into from
Jul 17, 2024

Conversation

kubaflo
Copy link
Contributor

@kubaflo kubaflo commented Jan 22, 2024 •
edited
Loading

Description of Change

The restrictions have been added to make sure that the custom icon is displayed only when the flyoutBehavior is equal FlyoutBehavior.Flyout on Android and iOS

Issues Fixed

Fixes #9037

Screen.Recording.2024-01-26.at.17.19.37.mov

@kubaflo kubaflo requested a review from a team as a code owner January 22, 2024 23:36
@ghost ghost added the community ✨ Community Contribution label Jan 22, 2024
@ghost
Copy link

ghost commented Jan 22, 2024

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.

@kubaflo kubaflo mentioned this pull request Jan 22, 2024
Closed
@jfversluis
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo force-pushed the flyoutIcon-when-flyout-disabled-fix branch from 6b3326c to c431cdd Compare January 26, 2024 16:20
@samhouts samhouts added the area-controls-shell Shell Navigation, Routes, Tabs, Flyout label Jan 31, 2024
@dustin-wojciechowski
Copy link
Contributor

dustin-wojciechowski commented Apr 25, 2024 •
edited
Loading

Looks good, tested and working on Android and ios.

Noticed that this already worked on Windows and needed no change in this PR as well:
20087_part1
20087_part2

@dustin-wojciechowski
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo
Copy link
Contributor Author

kubaflo commented Apr 25, 2024

Looks good, just noticing that no changes were needed for this to work on Windows 20087_part1 20087_part2

I unfortunately cannot say anything about the windows behavior because I don't have a windows device

@PureWeen
Copy link
Member

PureWeen commented Jun 1, 2024

/rebase

@PureWeen
Copy link
Member

PureWeen commented Jun 1, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

PureWeen
PureWeen requested changes Jun 1, 2024
View reviewed changes
src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellPageRendererTracker.cs Outdated
else if (String.IsNullOrWhiteSpace(text) && IsRootPage && _flyoutBehavior == FlyoutBehavior.Flyout)
{
icon = DrawHamburger();
if (image != null)
Copy link
Member

Choose a reason for hiding this comment

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

Would it make sense to move all the code below this to a separate method?

And then before we call LoadImage we can just check the flyout behavior? If it's set to Flyout then we just call the separate method with a null icon?

With this path, it seems like we're going to be loading an image that we never use.

@kubaflo kubaflo force-pushed the flyoutIcon-when-flyout-disabled-fix branch from 4f435d6 to cd35b7b Compare June 1, 2024 23:55
PureWeen
PureWeen previously approved these changes Jun 2, 2024
View reviewed changes
@PureWeen
Copy link
Member

PureWeen commented Jun 2, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen
Copy link
Member

PureWeen commented Jun 2, 2024

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

Rebased and added pending UITest snapshots.

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

jsuarezruiz
jsuarezruiz previously approved these changes Jun 13, 2024
View reviewed changes
@kubaflo kubaflo force-pushed the flyoutIcon-when-flyout-disabled-fix branch from 8187b05 to c9ebc82 Compare July 16, 2024 18:48
@kubaflo kubaflo requested a review from PureWeen July 16, 2024 18:50
@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@kubaflo kubaflo requested a review from PureWeen July 16, 2024 23:28
@PureWeen
Copy link
Member

/azp run

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen enabled auto-merge (squash) July 17, 2024 18:51
@PureWeen PureWeen disabled auto-merge July 17, 2024 23:44
@PureWeen
Copy link
Member

Test failures are unrelated

@PureWeen PureWeen merged commit 1b7bfe3 into dotnet:main Jul 17, 2024
95 of 97 checks passed
@samhouts samhouts added fixed-in-net9.0-nightly This may be available in a nightly release! fixed-in-8.0.80 and removed fixed-in-net9.0-nightly This may be available in a nightly release! labels Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PureWeen PureWeen PureWeen approved these changes

@jsuarezruiz jsuarezruiz jsuarezruiz left review comments

@dustin-wojciechowski dustin-wojciechowski dustin-wojciechowski left review comments

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

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

Assignees
No one assigned
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout community ✨ Community Contribution fixed-in-8.0.80 fixed-in-net9.0-nightly This may be available in a nightly release!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Custom FlyoutIcon visible although FlyoutBehavior set to disabled on Android an IOS
6 participants
@kubaflo @jfversluis @dustin-wojciechowski @PureWeen @jsuarezruiz @samhouts