Skip to content
This repository has been archived by the owner on May 1, 2024. It is now read-only.

Adds Shell Flyout Footer Template #6432

Merged
merged 23 commits into from
Oct 9, 2020

Conversation

SkyeHoefling
Copy link
Contributor

@SkyeHoefling SkyeHoefling commented Jun 6, 2019

Description of Change

Adds the Shell Flyout Footer Template implementation for Android meeting the following specs documented in the issue:

[Spec]

  • The footer should be fixed to the bottom of the flyout.
  • The menu list should not go behind it and obscure the menu items.
  • The footer should take any height.

Issues Resolved

API Changes

IShellController

Added:

View FlyoutFooter { get; }

Shell

Added:

View IShellController.FlyoutFooter { get; }
object FlyoutFooter { get; set; } // Bindable Property
DataTemplate FlyoutFooterTemplate { get; set; } // Bindable Property

There are several non-public methods added to this class, but the additions above are all the public changes that mirror FooterHeader

Platforms Affected

  • Core/XAML (all platforms)
  • Android

Behavioral/Visual Changes

Changed the ShellFlyoutTemplatedContentRenderer to render an additional ViewGroup that is the fixed FlyoutFooter which appears at the bottom of the Flyout.

FlyoutContent.axml

Added:

  • FrameLayout which is used to add the content of `FlyoutFooter.

ShellFlyoutTemplatedContentRenderer

Modified:

  • Updated the LoadView logic to properly insert the content from FlyoutFooter.
  • Once the content is inserted the LoadView logic calculates the height of the footer and applies the margin to the RecyclerView. This change ensures the ShellContent and FlyoutItem does not get rendered behind the FlyoutFooter

Before/After Screenshots

This is a new feature so there is no before screenshot. The after screenshot is a gif demonstrating the new feature.

shell_footer_demo

Testing Procedure

I updated the Xamarin.Forms.Sandbox project to use the new feature and completed my testing there.

  • Created Header and Footer
  • Added FlyoutItems and ShellContent
  • Added many items to force the RecyclerView to scroll and demonstrated the items are not hidden behind the FlyoutFooter

PR Checklist

  • Has automated tests
  • Rebased on top of the target branch at time of PR
  • Changes adhere to coding standard

Omitted Tests

There are currently no automated tests for this as the work focused on UI changes which can be difficult to write automation around. During the review process if we identify where tests are needed I am more than happy to add automated tests.

Questions

While working on this PR I noticed that ShellFlyoutContentRenderer didn't seem to be used and the
ShellFlyoutTemplatedContentRender render is used instead. Here is a code snippet that shows some stale code. I am not sure if this is a hold-over from an original attempt or needs to be removed or is there for a future implementation.

protected virtual IShellFlyoutContentRenderer CreateShellFlyoutContentRenderer()
{
return new ShellFlyoutTemplatedContentRenderer(this);
//return new ShellFlyoutContentRenderer(this, AndroidContext);
}

@samhouts samhouts added this to In Review in v4.2.0 Jun 6, 2019
@PureWeen PureWeen removed the request for review from StephaneDelcroix June 7, 2019 19:05
@PureWeen PureWeen added the DO-NOT-MERGE-!!! 🛑 This is in progress and needs to be updated before it can be merged. label Jun 7, 2019
@PureWeen PureWeen self-assigned this Jun 7, 2019
@PureWeen
Copy link
Contributor

PureWeen commented Jun 7, 2019

@ahoefling were you planning to work on the iOS side of this as well at all?

@SkyeHoefling
Copy link
Contributor Author

I can, but I don't have a business need for iOS at the moment as my project is targeting Android and that is what I built this for. As of right now I am not planning to add an iOS portion for this PR unless it is required to move this PR forward.

I can certainly start looking at the iOS implementation sometime in the next week or so.

@PureWeen
Copy link
Contributor

PureWeen commented Jun 7, 2019

@ahoefling no worries on not working on iOS. I was only asking just so duplicate work didn't occur at all. This will get us to the Footer Templates a lot faster!! I have some work on ios I've been doing over here https://github.com/xamarin/Xamarin.Forms/tree/shell_fixes to smooth out the flyout so I'll probably just end up merging your work into that and then finishing up the footer on ios as well

We'll have to get an iOS implementation done before we can merge the Android one but this will get us there a lot faster!!

@samhouts samhouts moved this from In Review to In Progress in v4.2.0 Jun 8, 2019
@SkyeHoefling
Copy link
Contributor Author

@PureWeen I'm glad this is going to make the implementation much easier and we will be able to use the work I started here. How should we proceed with getting this merged into your shell_fixes branch? Should I be expecting comments in a code review here or will you be taking ownership of this when you are ready to merge it into your branch?

Curious on our process forward with this.

Copy link
Contributor

@paymicro paymicro left a comment

Choose a reason for hiding this comment

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

LGTM

@PureWeen
Copy link
Contributor

@ahoefling

will you be taking ownership of this when you are ready to merge it into your branch?

Once the work for this is top of the queue then we'll just take what you've done here and add onto it. Since you've already done a lot of the guts then we should be able to move this up in priority a bit faster.

I have a bunch of existing work I need to merge over on ios so I'm hoping I can take what you did here and easily provide a footer 🤞

@samhouts samhouts removed this from In Progress in v4.2.0 Jul 4, 2019
@samhouts samhouts added the API-change Heads-up to reviewers that this PR may contain an API change label Jul 9, 2019
@samhouts samhouts added this to In Progress in v4.3.0 Jul 9, 2019
@samhouts samhouts removed this from In Progress in v4.3.0 Sep 3, 2019
@samhouts samhouts added Core and removed Core labels Sep 5, 2019
@samhouts samhouts added this to In Progress in v4.4.0 Sep 5, 2019
@samhouts samhouts removed this from In Progress in v4.4.0 Nov 2, 2019
@PureWeen
Copy link
Contributor

@ahoefling Sorry about the delay but can you rebase this?
Let me know if you don't have time and I can take it over

@xamarin xamarin deleted a comment from azure-pipelines bot Oct 9, 2020
@xamarin xamarin deleted a comment from azure-pipelines bot Oct 9, 2020
@xamarin xamarin deleted a comment from azure-pipelines bot Oct 9, 2020
@PureWeen
Copy link
Contributor

PureWeen commented Oct 9, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@xamarin xamarin deleted a comment from azure-pipelines bot Oct 9, 2020
@xamarin xamarin deleted a comment from azure-pipelines bot Oct 9, 2020
@PureWeen
Copy link
Contributor

PureWeen commented Oct 9, 2020

/azp run

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@PureWeen PureWeen merged commit 34b0936 into xamarin:5.0.0 Oct 9, 2020
vNext+1 (5.0.0) automation moved this from In Review to Done Oct 9, 2020
@samhouts samhouts added this to the 5.0.0 milestone Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a/shell 🐚 API-change Heads-up to reviewers that this PR may contain an API change blocker Issue blocks next stable release. Prioritize fixing and reviewing this issue. ControlGallery Core p/Android p/iOS 🍎 p/UWP proposal-accepted roadmap t/enhancement ➕
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

[Spec] Shell Flyout Footer Template
6 participants