[Android] TabbedPage: Bottom Tab Bar #1748

mikescandy · 2018-01-30T16:19:22Z

Description of Change

Adding a flag allowing a tabbed page to be rendered as a Bottom Navigation view on Android. Add the ability to set the tint color of unselected and selected items.

Bugs Fixed

fixes #1675

API Changes

Added:

AndroidSpecific.SetToolbarPlacement // Top, Bottom, Default

On<Android>().SetToolbarPlacement(ToolbarPlacement.Bottom);

<?xml version="1.0" encoding="utf-8"?>
<TabbedPage xmlns="http://xamarin.com/schemas/2014/forms" 
    xmlns:android="clr-namespace:Xamarin.Forms.PlatformConfiguration.AndroidSpecific;assembly=Xamarin.Forms.Core"
    android:TabbedPage.ToolbarPlacement="Bottom"
...

AndroidSpecific.GetMaxItemCount // returns 5 for bottom and maxvalue for top
AndroidSpecific.SetBarItemColor // sets the tint color for the icons (and the text color if text color is default)
AndroidSpecific.SetBarSelectedItemColor //sets the tint color for the selected icon (and the text color if the text color is default)
In the Android TabbedPageRenderer added

protected virtual ColorStateList GetItemTextColorStates()
protected virtual ColorStateList GetItemIconTintColorState()

Allowed the user to define there own ColorStateList relationship as they pertain to the Tabs

PR Checklist

Has tests (if omitted, state reason in description)
Rebased on top of master at time of PR
Changes adhere to coding standard
Consolidate commits as makes sense

dnfclas · 2018-01-30T16:19:33Z

All CLA requirements met.

jassmith · 2018-01-31T05:19:35Z

Hey thank you! Unfortunately I am not in a position to actually run this for a couple days. I would really appreciate some screenshots however just so I can see how it looks. I will give the code a detailed review tomorrow, however a quick glance looks good.

jassmith · 2018-01-31T05:25:28Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs


-			if (width > 0 && height > 0)
+				((IPageController)Element).ContainerArea = new Rectangle(0, 0, _relativeLayout.MeasuredWidth, _relativeLayout.GetChildAt(0).MeasuredHeight);


Without having run it or really grokked everything in here yet this seems wrong. The ContainerArea should be the region of the TabbedPage that does NOT contain the tabs itself. So in general this would be 0, 0, r - l, b - t - bottomTabBarHeight

that's absolutely right. I was doing a bit more test and trying to understand that code a bit better and I realized the same. I'll push a better version, along with some screenshots

(the OnLayout override is the most mysterious to me right now)

mikescandy · 2018-01-31T14:56:56Z

mikescandy · 2018-01-31T15:03:20Z

I have a few questions on how to handle the colors for icons and text, in terms of selected/not selected/disabled.
Also, we should probably disable the swipe gesture.

StephaneDelcroix · 2018-01-31T19:30:00Z

I'd avoid using BottomNavigation wording. if the default changes in the future on Android, that property would become awkward.

I'd expose this property with a name like xxPositionxxx and it's value being an enum Default, Top, Bottom

hartez · 2018-01-31T19:30:52Z

See also: UWP toolbar placement: https://github.com/xamarin/Xamarin.Forms/blob/74cb5c4a97dcb123eb471f6b1dffa1267d0305aa/Xamarin.Forms.Core/PlatformConfiguration/WindowsSpecific/ToolbarPlacement.cs

mikescandy · 2018-01-31T21:47:27Z

While we are exposing it using the same element (tabbedpage), tabs and bottomnavigation view behave quite differently. My initial implementation was actually using a different control, but then I saw the enhancement proposal and I merged my implementation inside the tabbedpage, which simplified things a lot.
The bottomnavigationview support a max number of 5 elements, while tabs are effectively unlimited.
I would interpret the placement as same control in different position. Not sure really.

MaikuB · 2018-02-04T04:20:33Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

-			else if (newTextColor.IsDefault && _defaultColor.HasValue && currentColor != _defaultColor)
-				_tabLayout.SetTabTextColors(_defaultColor.Value, _defaultColor.Value);
+			int selectedIndex = item.Order;
+			if (_bottomNavigationView.SelectedItemId != item.Order && Element.Children.Count > selectedIndex && selectedIndex >= 0)


i think the first condition is meant to be _bottomNavigationView.SelectedItemId != item.ItemId

MaikuB · 2018-02-04T04:29:08Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

 using Android.Views;
 using Xamarin.Forms.Internals;
 using Xamarin.Forms.PlatformConfiguration.AndroidSpecific;

 namespace Xamarin.Forms.Platform.Android.AppCompat
 {
-	public class TabbedPageRenderer : VisualElementRenderer<TabbedPage>, TabLayout.IOnTabSelectedListener, ViewPager.IOnPageChangeListener, IManageFragments
+	public class TabbedPageRenderer : VisualElementRenderer<TabbedPage>, TabLayout.IOnTabSelectedListener, ViewPager.IOnPageChangeListener, IManageFragments, BottomNavigationView.IOnNavigationItemSelectedListener


I'd suggest making modifications to OnPagePropertyChanged to handle when a page's title changes (e.g. via data binding). The current renderer already does this when the native element is a TabLayout. I think the code you'd need for the BottomNavigationView is something like

var page = (Page)sender;
var index = Element.Children.IndexOf(page);
var tab = _bottomNavigationView.Menu.GetItem(index);
tab.SetTitle(page.Title);

MaikuB · 2018-02-04T04:30:42Z

i'm interested in this being officially supported by Xamarin.Forms as well. Added some comments/suggestions to the PR

jassmith · 2018-02-15T23:54:38Z

retriggering builds

jassmith · 2018-02-16T00:22:03Z

Can you please update the docs? There is a script to do it. Without the update we wont get CI results

mikescandy · 2018-02-16T11:15:56Z

I'm off for a couple of days, will do some work over the weekend

jassmith · 2018-02-22T07:20:32Z

Xamarin.Forms.Core/PlatformConfiguration/AndroidSpecific/TabsPlacement.cs

+{
+	public enum TabsPlacement
+	{
+		/// <summary>


Please dont use inline docs, they wont get properly updated. Use the mdoc files instead.

I copy/pasted from the WindowsSpecific\ToolbarPlacement.cs file. Anyway, should I just delete the comments? Also, I couldn't find the mdoc files :(

@mikescandy yea just delete the comments.. Since you rebased with master you won't need to run the docs file that generates docs anymore. Just make sure the API Changes part of the PR has everything

PureWeen · 2018-03-06T17:50:37Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs


-			int currentColor = _tabLayout.TabTextColors.DefaultColor;
+        if (!_defaultColor.HasValue)


Spaces => Tabs

Fixed in next commit.

PureWeen · 2018-03-06T17:51:44Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

 			}

-			base.OnLayout(changed, l, t, r, b);
+      base.OnLayout(changed, l, t, r, b);


Spaces => Tabs

Fixed in next commit.

PureWeen · 2018-03-06T18:57:30Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

+						viewPagerParams.AddRule(AWidget.LayoutRules.Above, _bottomNavigationView.Id);
+
+						FormsViewPager pager =
+							_viewPager =


Could this code be moved to a general CreateFormsViewPager method? Then both the bottom and top navbar would just call that? The way these new up looks to be the same between the two

Fixed in next commit.

PureWeen · 2018-03-06T19:28:22Z

Xamarin.Forms.Core/PlatformConfiguration/AndroidSpecific/TabsPlacement.cs

@@ -0,0 +1,17 @@
+namespace Xamarin.Forms.PlatformConfiguration.AndroidSpecific
+{
+	public enum TabsPlacement


Add Default here as well just in case Android changes what Default means in the future

Fixed in next commit. I don't think this has an Android equivalent really, but agree it's right to put a Default here.

PureWeen · 2018-03-06T19:29:24Z

Xamarin.Forms.Core/PlatformConfiguration/AndroidSpecific/TabbedPage.cs

+
+		public static void SetTabsPlacement(BindableObject element, TabsPlacement value)
+		{
+			element.SetValue(TabsPlacementProperty, value);


Can we throw an explicit exception here if the user tries to toggle between Top <-> Bottom after the page has already been rendered? This way it's clear to the user that changing the placement after it's been realized isn't supported

Help here: as this is an enum, my understanding is that this will always have a value (Default), so I probably should not throw on the first value change. Where should I track the value change?

If you rebase to master you'll see a property on BindableObject now that lets you test if something has been changed from the default
https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/BindableObject.cs#L55

Which should be sufficient

My other thought would be to add a Changing event to the bindable property. Then inside the Tab Renderer once the layout call happens it could set a local value indicating this is how it is on the layout. The changing event could then check that... But that's probably overkill and just checking IsSet should be enough

I'll try the rebase. I'm not the best at using GIT, let's see what happens...

Should be done. Please check if the exception message makes sense.

@mikescandy Hey mike, looks like your pretty much done with this, when can we mortals expect this as a nuget-package? I suppose its going into pre-release.

Error message seems to work just remember to set Default as the defaultValue

public static readonly BindableProperty TabsPlacementProperty = BindableProperty.Create("TabsPlacement", typeof(TabsPlacement), typeof(TabbedPage), TabsPlacement.Top);

And then at this point Default and Top will just cause the same behavior

PureWeen · 2018-03-06T19:36:37Z

Xamarin.Forms.Controls/CoreGallery.cs

@@ -170,6 +170,69 @@ public void TestReorderTabs ()
 #endif
 	}

+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Github, 1675, "Android: TabbedPage: Bottom Tab Bar", PlatformAffected.Android)]


Can you add some Android specific UITests as well modeled after

public class TabbedPageTests : TestTabbedPage

So that we can generate some screen shots of the bottom nav bar and just have some automated ui tests for this

PureWeen · 2018-03-06T20:53:42Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

+				bottomNavigationView.Menu.Add(0, i, i, child.Title).SetEnabled(child.IsEnabled);
+			}
+
+			if (Element.Children.Count > 0)


Because of how this is implemented if I'm not on the first tab then when I remove a page it'll just navigate me back to the first page. Can this be setup to just remain on the Current Page if that page wasn't removed? Something like

for (var i = 0; i < Element.Children.Count; i++) { Page child = Element.Children[i]; var menuItem = bottomNavigationView.Menu.Add(0, i, i, child.Title).SetEnabled(child.IsEnabled); if(Element.CurrentPage == child) { bottomNavigationView.SelectedItemId = menuItem.ItemId; } } if (Element.Children.Count > 0) { if(Element.CurrentPage == null || !Element.Children.Contains(Element.CurrentPage)) Element.CurrentPage = Element.Children[0]; }

Also is it necessary to clear out and rebuild the whole menu each time an item is removed/added?

That will do. Not sure how not to rebuild the menu. I should keep some kind of dictionary linking menu item ids and child pages... not really sure

Since the method is called by OnChildrenCollectionChanged could the NotifyCollectionChangedEventArgs be used for the index of the removed page?

PureWeen · 2018-03-06T21:05:15Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

+				int newTextColorArgb = newTextColor.ToAndroid().ToArgb();
+				var color = newTextColor.ToAndroid();
+				color.A = 128;
+				int newTextColorArgbFade = color.ToArgb();


What does this do? It doesn't seem like this is used later for anything

Garbage. Also, renaming color -> semiTransparentColor as it's more meaningful and explains what I'm doing there

@mikescandy sounds good

I'm also thinking we should either remove setting the ItemIconTintList or add a platform property BarIconTintColor that can be applied to either AppBar. If setting that Tint Color on a top app bar is tricky though maybe just have BottomBarIconTintColor

PureWeen · 2018-03-06T22:15:16Z

Xamarin.Forms.Platform.Android/AppCompat/TabbedPageRenderer.cs

+				if (!newTextColor.IsDefault && currentColor != newTextColorArgb)
+				{
+					_bottomNavigationView.ItemTextColor = new ColorStateList(new[] { sChecked, sDefault }, new[] { newTextColorArgb, color });
+					_bottomNavigationView.ItemIconTintList = new ColorStateList(new[] { sChecked, sDefault }, new[] { newTextColorArgb, color });


Do we want the BarTextColor to also set the Tint? Can we make the Tint Color a different settable property? Then we could also try applying said tint setting to the top bar as well

rmarinho · 2018-03-28T09:35:55Z

hey @mikescandy you need to rebase and remove docs, it's not needed anymore. thanks

* Adding bottom navigation support to tabbed page

* Adding bottom navigation support to tabbed page * Adding a bottom navigation page to the gallery * Set the default behavior to false to avoid introducing a breaking change * Fixing onlayout, handling collection change and improving color management

Renaming semi transparent color variable, throw when changing tabs placement after setting it

PureWeen · 2018-05-08T22:23:27Z

build --uitests

PureWeen · 2018-05-09T15:30:39Z

Test failure is being misreported. All tests ran fine in appcenter

Fabone248 · 2018-06-18T00:04:17Z

Hello,

I am using the tool bar placement function to set tabs at the bottom of the screen, but I noticed that if we have more than 3 tabs, then we don't see the titles anymore and the selected icon kind of pushes the other icons away. I'd like to know if there is a way to deactivate this behavior and have all titles showing up and the unselected icons not moving at all?

Thank you for your help.

hunii · 2018-07-30T22:32:21Z

I like to know the above question too...

Roshek · 2018-07-31T07:39:18Z

@hunii @Fabone248 AFAIK you can currently only do this by adding a custom renderer. See gist here: https://gist.github.com/LynoDesu/64904b6d143892cf14a60a32798a36bb

If you have more questions you can also read through #1675

Fabone248 · 2018-07-31T21:19:15Z

@Roshek Thank you for your reply, I will have a look :)

pauldipietro added this to New in Triage Jan 30, 2018

PureWeen moved this from New to Proposal in Triage Jan 30, 2018

PureWeen added t/enhancement ➕ p/Android F100 labels Jan 30, 2018

jassmith reviewed Jan 31, 2018

View reviewed changes

MaikuB reviewed Feb 4, 2018

View reviewed changes

samhouts changed the title ~~Android: TabbedPage: Bottom Tab Bar~~ [Android] TabbedPage: Bottom Tab Bar Feb 7, 2018

rmarinho requested a review from jassmith February 9, 2018 10:44

samhouts added this to In Progress in Enhancements Feb 12, 2018

samhouts added this to Ready in v3.1.0 via automation Feb 12, 2018

samhouts requested a review from PureWeen February 12, 2018 18:45

samhouts moved this from Ready to In Review in v3.1.0 Feb 12, 2018

rmarinho removed this from Proposal in Triage Feb 14, 2018

jassmith suggested changes Feb 22, 2018

View reviewed changes

PureWeen suggested changes Mar 6, 2018

View reviewed changes

samhouts mentioned this pull request Mar 23, 2018

Add support for Android bottom navigation bar in Xamarin.Forms #1400

Closed

PureWeen changed the base branch from master to 3.1.0 May 3, 2018 21:12

xamarin-release-manager added the API-change Heads-up to reviewers that this PR may contain an API change label May 7, 2018

mikescandy and others added 8 commits May 7, 2018 15:54

Bottom navigation page (#2)

0c99d0d

* Adding bottom navigation support to tabbed page

Bottom navigation page (#3)

8201447

* Adding bottom navigation support to tabbed page * Adding a bottom navigation page to the gallery * Set the default behavior to false to avoid introducing a breaking change * Fixing onlayout, handling collection change and improving color management

Changing the bindable property to TabsPlacement

3acba7a

Handling page title change for bottom navigation view

4e41206

Fix using itemid instead of order

a2d6c81

First round of code review changes

ffee6ea

More code review changes

2a37277

Renaming semi transparent color variable, throw when changing tabs placement after setting it

fixes #1675

27c6da2

samhouts moved this from In Progress to In Review in v3.1.0 May 7, 2018

xamarin-release-manager added the API-change Heads-up to reviewers that this PR may contain an API change label May 8, 2018

jassmith approved these changes May 8, 2018

View reviewed changes

PureWeen merged commit 3cc4232 into xamarin:3.1.0 May 9, 2018

v3.1.0 automation moved this from In Review to Done May 9, 2018

Enhancements automation moved this from In Progress to Done May 9, 2018

samhouts removed this from Done in Enhancements Jun 12, 2018

samhouts added this to the 3.2.0 milestone Sep 11, 2018

samhouts removed this from Done in v3.1.0 Sep 11, 2018

samhouts added this to Done in v3.2.0 Sep 11, 2018

samhouts removed this from the 3.2.0 milestone Sep 12, 2018

samhouts added this to Done in v3.1.0 Sep 12, 2018

samhouts removed this from Done in v3.2.0 Sep 12, 2018

samhouts added the community-sprint label Nov 2, 2018


		if (width > 0 && height > 0)
		((IPageController)Element).ContainerArea = new Rectangle(0, 0, _relativeLayout.MeasuredWidth, _relativeLayout.GetChildAt(0).MeasuredHeight);


		int currentColor = _tabLayout.TabTextColors.DefaultColor;
		if (!_defaultColor.HasValue)

[Android] TabbedPage: Bottom Tab Bar #1748

[Android] TabbedPage: Bottom Tab Bar #1748

Conversation

mikescandy commented Jan 30, 2018 • edited by davidortinau

Description of Change

Bugs Fixed

API Changes

PR Checklist

dnfclas commented Jan 30, 2018 • edited

jassmith commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikescandy commented Jan 31, 2018 • edited by PureWeen

mikescandy commented Jan 31, 2018 • edited

StephaneDelcroix commented Jan 31, 2018

hartez commented Jan 31, 2018

mikescandy commented Jan 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaikuB commented Feb 4, 2018

jassmith commented Feb 15, 2018

jassmith commented Feb 16, 2018

mikescandy commented Feb 16, 2018 via email • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmarinho commented Mar 28, 2018

PureWeen commented May 8, 2018

PureWeen commented May 9, 2018

Fabone248 commented Jun 18, 2018

hunii commented Jul 30, 2018

Roshek commented Jul 31, 2018

Fabone248 commented Jul 31, 2018

mikescandy commented Jan 30, 2018 •

edited by davidortinau

dnfclas commented Jan 30, 2018 •

edited

mikescandy commented Jan 31, 2018 •

edited by PureWeen

mikescandy commented Jan 31, 2018 •

edited

mikescandy commented Feb 16, 2018 via email •

edited