-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Conversation
@ahoefling were you planning to work on the iOS side of this as well at all? |
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. |
@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!! |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 🤞 |
@ahoefling Sorry about the delay but can you rebase this? |
ac583b2
to
ddd581b
Compare
/azp run |
No pipelines are associated with this pull request. |
/azp run |
No pipelines are associated with this pull request. |
Description of Change
Adds the Shell Flyout Footer Template implementation for Android meeting the following specs documented in the issue:
[Spec]
Issues Resolved
API Changes
IShellController
Added:
Shell
Added:
There are several non-public methods added to this class, but the additions above are all the public changes that mirror
FooterHeader
Platforms Affected
Behavioral/Visual Changes
Changed the
ShellFlyoutTemplatedContentRenderer
to render an additionalViewGroup
that is the fixedFlyoutFooter
which appears at the bottom of the Flyout.FlyoutContent.axml
Added:
FrameLayout
which is used to add the content of `FlyoutFooter.ShellFlyoutTemplatedContentRenderer
Modified:
LoadView
logic to properly insert the content fromFlyoutFooter
.LoadView
logic calculates theheight
of the footer and applies the margin to theRecyclerView
. This change ensures theShellContent
andFlyoutItem
does not get rendered behind theFlyoutFooter
Before/After Screenshots
This is a new feature so there is no before screenshot. The after screenshot is a gif demonstrating the new feature.
Testing Procedure
I updated the
Xamarin.Forms.Sandbox
project to use the new feature and completed my testing there.RecyclerView
to scroll and demonstrated the items are not hidden behind theFlyoutFooter
PR Checklist
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 theShellFlyoutTemplatedContentRender
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.Xamarin.Forms/Xamarin.Forms.Platform.Android/Renderers/ShellRenderer.cs
Lines 166 to 170 in f0a33ee