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

[TIMOB-26354] Android: Refactor TabGroup and introduce new style #10358

Merged
merged 40 commits into from Dec 11, 2018

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Sep 28, 2018

JIRA: https://jira.appcelerator.org/browse/TIMOB-26354

Description:
Refactor the implementation of TabGroup - remove deprecated ActioBar tabs UI and replace it with TabLayout. Introduce a new style for using BottomNavigationView. A more detailed description will be available once I am done with everything.

Currently still trying to fix the removal of tabs before opening the TabGroup by simplifying the relation between the tab proxies added and their TiUiAbstractTab counterparts.

Update:
How things work after the refactoring:

  • all the code related to the deprecated ActionBar Tabs UI has been removed - that way all the problems with themes originating from ActionBar are no longer present. For instance now TabGroup can be used with a Theme.AppCompat.NoTitleBar and children of it without issues;
  • handling of the ViewPager that is used to show the Tabs is moved to TiUIAbstractTabGroup;
  • TiUIAbstractTabGroup provides an interface for communication with different "controllers";
  • this allows us to have the separate styles as different implementations making it easier to support and introduce new styles;
  • backwards compatibility with the JS code for the TabGroup implementation before this refactoring. Not defining a style at creation falls back to using the DEFAULT style which is as close as possible the ActionBar UI, but following the Material Design Guidelines ( one difference is the position of the icon relative to the title );
  • fixes the behavior of some properties ( tabsBackgroundColor, tabsFocusedBackgroundColor ), adds some new properties ( style, shiftMode ) and brings parity with iOS for other ( titleColor, activeTitleColor );

Note: The constants for style will be used from the same namespace for the TabbedBar component. I will change them in the PR for it once this gets the green light for merging.

Tickets that should be resolved by this refactoring:

Test case:
app.js

var mainWindow = Ti.UI.createWindow({
		layout: 'vertical'
	}),
	buttonDefault = Ti.UI.createButton({
		title: 'Default'
	}),
	buttonBottom = Ti.UI.createButton({
		title: 'Bottom'
	}),
	winDefaultA = Ti.UI.createWindow({
		backgroundColor: 'red'
	}),
	winDefaultB = Ti.UI.createWindow({
		backgroundColor: 'green'
	}),
	winDefaultC = Ti.UI.createWindow({
		backgroundColor: 'blue'
	}),
	tabDefaultA = Ti.UI.createTab({
		title: 'Tab A',
		window: winDefaultA
	}),
	tabDefaultB = Ti.UI.createTab({
		title: 'Tab B',
		window: winDefaultB
	}),
	tabDefaultC = Ti.UI.createTab({
		title: 'Tab C',
		window: winDefaultC,
		backgroundColor: 'cyan',
		backgroundFocusedColor: 'magenta',
		titleColor: 'red',
		activeTitleColor: 'blue'
	}),
	tabGroupDefault = Ti.UI.createTabGroup({
		// for backwards compatibility if no style is defined, the component falls back to the default one
		style: Ti.UI.Android.TABS_STYLE_DEFAULT,
		tabs: [tabDefaultA, tabDefaultB, tabDefaultC],
		tabsBackgroundColor: '#88AAAAAA',
		tabsBackgroundSelectedColor: '#AAAAAA'
	}),
	winBottomA = Ti.UI.createWindow({
		backgroundColor: 'red'
	}),
	winBottomB = Ti.UI.createWindow({
		backgroundColor: 'green'
	}),
	winBottomC = Ti.UI.createWindow({
		backgroundColor: 'blue'
	}),
	tabBottomA = Ti.UI.createTab({
		title: 'Tab 1',
		window: winBottomA
	}),
	tabBottomB = Ti.UI.createTab({
		title: 'Tab 2',
		window: winBottomB,
	}),
	tabBottomC = Ti.UI.createTab({
		title: 'Tab 3',
		window: winBottomC,
		backgroundColor: 'cyan',
		backgroundFocusedColor: 'magenta',
		titleColor: 'red',
		activeTitleColor: 'blue'
	})
	tabGroupBottom = Ti.UI.createTabGroup({
		style: Ti.UI.Android.TABS_STYLE_BOTTOM_NAVIGATION,
		tabs: [tabBottomA, tabBottomB, tabBottomC],
		// The following two properties override the colors from TabGroup
		tabsBackgroundColor: '#88994444',
		tabsBackgroundSelectedColor: '#883333',
		// Comment the row below or set it to true to see the difference
		shiftMode: false
	}),
	buttonDisableTabNavigation = Ti.UI.createButton({
		title: 'Disable Tab Navigation'
	}),
	flagTabNavigationDisabled = false,
	buttonSetActiveTab = Ti.UI.createButton({
		title: 'Go to Tab A'
	});

buttonDisableTabNavigation.addEventListener('click', function () {
	flagTabNavigationDisabled = !flagTabNavigationDisabled;
	tabGroupDefault.disableTabNavigation(flagTabNavigationDisabled);
});
winDefaultB.add(buttonDisableTabNavigation);

buttonSetActiveTab.addEventListener('click', function () {
	tabGroupDefault.setActiveTab(tabDefaultA);
});
winDefaultC.add(buttonSetActiveTab);

buttonDefault.addEventListener('click', function () {
	tabGroupDefault.open();
});

buttonBottom.addEventListener('click', function () {
	tabGroupBottom.open();
});

mainWindow.add(buttonDefault);
mainWindow.add(buttonBottom);
mainWindow.open();

tabGroupDefault.addEventListener('open', function () {
	Ti.API.info('Default TabGroup OPEN event fired');
});
tabGroupDefault.addEventListener('close', function () {
	Ti.API.info('Default TabGroup CLOSE event fired');
});
tabGroupDefault.addEventListener('focus', function () {
	Ti.API.info('Default TabGroup FOCUS event fired');
});
tabGroupDefault.addEventListener('blur', function () {
	Ti.API.info('Default TabGroup blur event fired');
});
tabDefaultA.addEventListener('focus', function () {
	Ti.API.info('Default Tab A FOCUS event fired');
});
tabDefaultA.addEventListener('blur', function () {
	Ti.API.info('Default Tab A BLUR event fired');
});
tabDefaultA.addEventListener('selected', function () {
	Ti.API.info('Default Tab A SELECTED event fired');
});
tabDefaultA.addEventListener('UNSELECTED', function () {
	Ti.API.info('Default Tab A UNSELECTED event fired');
});

Note: I haven't added a new example in the docs. I was thinking the test case from here should do, but maybe it is not in the best format to go there (too long?).

…s UI and replacing it with TabLayout. Introducing a new style.
@hansemannn
Copy link
Collaborator

Yey! Will try it out asap.

@jquick-axway
Copy link
Contributor

Cool. I like where this is heading. 😄

I'm curious about the ripple effect you are adding. Does it work like the Google Play tabs? Where tapping a tab changes the background color of the whole tab bar with that circle scaling effect?

Oh and the TiUIAbstractTab class should probably stay abstract eh?

Remove the Lifecycle Interface implementation from TiUIAbstractTabGroup.
Fix removing of Tabs before AND after opening the TabGroup.
Add titleColor and activeTitleColor properties of Tab parity for Android.
Add support for disabling shiftMode for BottomNavigationView (not yet exposed in the JS).
Prevent double selection of items in the TabGroup controller.
@build build added the docs label Oct 3, 2018
@ypbnv
Copy link
Contributor Author

ypbnv commented Oct 3, 2018

Yes, ActionBar tabs had the ripple effect by default and since I am messing with the background drawables to support different colors for selected/unselected states I needed to add it manually.

TiUIAbstractTab is not abstract anymore so I renamed it in the most recent commit.

Copy link
Contributor

@jawa9000 jawa9000 left a comment

Choose a reason for hiding this comment

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

This yml update looks good to me.

Clean up listener for tab changes.
Fix focus/blur and select/unselect events for TabGroup and Tabs.
Optimize the handling of tabs swiping.
Fix disabling of tab navigation.
Format code.
Guard agains API level.
Guard for closing the group before drawing it.
Cleanup a bit of code and imports.
Lint unit tests and format code.
Add docs.
@build
Copy link
Contributor

build commented Oct 17, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@hansemannn
Copy link
Collaborator

Quick note: You could add even more parity by exposing the badge property via:

bottomNavigation.setNotification(notification, bottomNavigation.getItemsCount() - 1);

@lokeshchdhry
Copy link
Contributor

lokeshchdhry commented Nov 20, 2018

@ypbnv , I see that tab1.png & tab2.png inside assets/images/ are not resized for me & it appears tiny on the default tab bar. Bottom bar is fine.

@ypbnv
Copy link
Contributor Author

ypbnv commented Nov 28, 2018

@jquick-axway I did some testing and it seems that prior to the refactoring the image was still passed as a drawable with its raw dimensions. Here a two screenshots for the same project with the same resources. I have intentionally used images with different dimensions - one is 30x30 and other 60x60 pixels:

This PR
refactored

7.5.0.GA
current

I also thought that they were scaled according to the device, but I suppose the difference was more noticeable due to the new layout. That opens the question if we still want to scale them in the refactored code?
Technically it will be a change to the behavior before that, but I am OK if we decide to add it. I tried a couple of things to deal with it, but so far the best I got is related to a change in one of the layout files (https://github.com/appcelerator/titanium_mobile/blob/master/android/modules/design/res/layout/design_layout_tab_icon.xml) in the Design library we package with the app. Personally, I am not a big fan of this solution, because it is not future proof.
I think somewhere in the process of updating the ImageView in this file with a new source is the problem, because this layout does not have the same behavior in a new clean native Android project, provided with the same source. So I will keep digging if we want to scale the source.

@jquick-axway
Copy link
Contributor

@ypbnv, I'm sorry man. You are exactly right. The old tab group handling did not scale the tab icons. My bad.
Titanium 7.5.0
tabgroupicononly_7 5 0
Titanium 8.0.0
tabgroupicononly_8 0 0

@jquick-axway
Copy link
Contributor

@lokeshchdhry, this PR is ready for you.

@lokeshchdhry
Copy link
Contributor

Passing final FR (first was done earlier before CR) as all CR comments seems to be looked at & appropriate changes made.

Studio Ver: 5.1.2.201810301430
SDK Ver: 8.0.0 local build
OS Ver: 10.14
Xcode Ver: Xcode 10.1
Appc NPM: 4.2.13
Appc CLI: 7.0.8
Daemon Ver: 1.1.3
Ti CLI Ver: 5.1.1
Alloy Ver: 1.13.4
Node Ver: 8.9.1
NPM Ver: 5.5.1
Java Ver: 10.0.2
Devices: ⇨ google Nexus 5 (Android 6.0.1)
⇨ google Pixel (Android 9)
Emulator: ⇨ Android 4.1.2

@tidev tidev deleted a comment from hansemannn Dec 5, 2018
@juliengatt
Copy link

hi, how to change icon color?
Thanks

@jquick-axway
Copy link
Contributor

@juliengatt , Titanium does not support tinting the tab icons on Android. So, the icon PNG you assign to the tab is displayed as-is.

@hansemannn
Copy link
Collaborator

We could use itemIconTint together with the ColorStateList for the checked and unchecked state:

<selector xmlns:android="http://schemas.android.com/apk/res/android">
  <item
      android:state_checked="true"
      android:color="@color/colorPrimary" />
  <item
      android:state_checked="false"
      android:color="@color/grey" />
 </selector>

Should be possible via XML.

@jquick-axway
Copy link
Contributor

Yes, it is possible to do it natively on Android, but Titanium does not currently support it.

@hansemannn
Copy link
Collaborator

But shouldn't it be possible with global styling. In any case, it's one of the last tabgroup parity issues effecting devs in real life use cases

@jquick-axway
Copy link
Contributor

But shouldn't it be possible with global styling?

On Android, you would normally do this via a drawable in XML. In that drawable's XML, you can define the different states. I think our internal code will load a drawable as-is in this case (not assume it's a BitmapDrawable) when loading from a res/drawable folder. So, perhaps it's theoretically possible to do it now.

@juliengatt
Copy link

If already possible, an example would be greatly appreciated!

@jquick-axway
Copy link
Contributor

You would need to create a "selector" in XML kind of like what is shown in the link below. This XML file would go under your project's ./platform/android/res/drawable folder. Let's say you call it "my_tab_icon1.xml".
https://stackoverflow.com/a/49854402

And then you would assign the above to the tab's icon as follows...

Ti.UI.createTab({
	icon: "android.resource://" + Ti.App.id + "/drawable/my_tab_icon1",
});

Note that I haven't tested the above. But "something" like that should theoretically work. You'll have to figure it out from there.

I've also written up a feature request ticket below.
https://jira.appcelerator.org/browse/TIMOB-26734

@juliengatt
Copy link

juliengatt commented Jan 23, 2019

@jquick-axway I haven't been able to make it work. I tried the following:

Ti.UI.createTab({
	icon: "android.resource://" + Ti.App.id + "/drawable/my_tab_icon1"
        //Tried this too: Ti.App.Android.R.drawable.my_tab_icon1
});

my_tab_icon1.xml and selected.png & unselected.png in drawable folder

<selector xmlns:android="http://schemas.android.com/apk/res/android">
  <item android:drawable="@drawable/selected" android:state_selected="true"/>
   <item android:drawable="@drawable/unselected" android:state_selected="false"/>
</selector>

I have found out that whatever icon color I add to the folder, the selected icon color will always be the opposite. I don't think this should be the expected behavior.

Please advise.
Thanks

@juliengatt
Copy link

We could use itemIconTint together with the ColorStateList for the checked and unchecked state:

<selector xmlns:android="http://schemas.android.com/apk/res/android">
  <item
      android:state_checked="true"
      android:color="@color/colorPrimary" />
  <item
      android:state_checked="false"
      android:color="@color/grey" />
 </selector>

Should be possible via XML.

@hansemannn did you make it work?
Thanks

@juliengatt
Copy link

I have seen the following ticket but not able to reproduce:
https://jira.appcelerator.org/browse/TIMOB-3179

@juliengatt
Copy link

Possible to use font awesome?

@Brianggalvez
Copy link
Contributor

Brianggalvez commented Feb 1, 2019

@juliengatt try with a custom theme

customTheme.xml

<resources>

  <!-- Base application theme. -->
  <style name="customTheme" parent="Theme.AppCompat.Light">
    <item name="itemIconTint">@drawable/nav_item_color_state</item>
  </style>
</resources>

drawable/nav_item_color_state.xml

<?xml version="1.0" encoding="utf-8"?>
<selector xmlns:android="http://schemas.android.com/apk/res/android">
    <item android:state_checked="true" android:color="@color/colorAccent"/>
    <item android:state_checked="false" android:color="@color/colorPrimaryDark"/>
</selector>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants