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

fix(android/ios): amend Tab tintColor and activeTintColor #11590

Closed
wants to merge 36 commits into from

Conversation

garymathews
Copy link
Contributor

@garymathews garymathews commented Apr 2, 2020

Android

  • Amend both Ti.UI.Tab properties titleColor and activeTitleColor

Android/iOS

  • Implement tintColor and activeTintColor
TEST CASE
const tabGroup = Ti.UI.createTabGroup({
	// style: Ti.UI.Android.TABS_STYLE_BOTTOM_NAVIGATION
});

const tabWin_home = Ti.UI.createWindow({
   	title: 'Home',
   	backgroundColor: 'white'
});
const tab_home = Ti.UI.createTab({
   	icon: 'home.png',
   	title: 'Home',
   	titleColor: 'red',
   	activeTitleColor: 'blue',
   	tintColor: 'red',
   	activeTintColor: 'blue',
   	window: tabWin_home
});

const tabWin_chat = Ti.UI.createWindow({
	title: 'Chat',
	backgroundColor: 'white'
});
const tab_chat = Ti.UI.createTab({
	icon: 'chat.png',
	title: 'Chat',
	titleColor: 'red',
	activeTitleColor: 'blue',
	tintColor: 'red',
	activeTintColor: 'blue',
	window: tabWin_chat
});

tabGroup.addTab(tab_home);
tabGroup.addTab(tab_chat);
tabGroup.open();
  • Tab titles and text should show the appropriate colors

TIMOB-27830
TIMOB-27831
TIMOB-27847
TIMOB-27911

since: {iphone: "3.1.3", ipad: "3.1.3", android: "9.1.0"}
platforms: [iphone, ipad, android]

- name: activeTintColor
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is available on iOS already. I think tintColor is the active tint and there is an additional one for the unselected items.

Copy link
Collaborator

Choose a reason for hiding this comment

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

'TabGroup': {
	activeTabIconTint: 'red',
	tabsTintColor: 'blue',
	unselectedItemTintColor: 'green'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we have a parity issue then. We should select the property most in keeping with our typical property naming/defaults as the proper one, deprecate the other(s) and ensure parity cross-platform.

The issue here seems to be that the relevant properties do already exist on the TabGroup level, but the new implementation is at the Tab level. I can actually see a use case where you'd prefer to set some tab group default for colors but say have individual tabs use different colors when they're active, so I do think they both have merit.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have a ticket written for activeTabIconTint here...
https://jira.appcelerator.org/browse/TIMOB-26734

If I remember right, Android didn't have a native API that supported this for top tabs at the time. It was only possible via style/theme XML settings. But there may be new APIs we can leverage now that we've migrated to AndroidX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's kind of a mess:

TabGroup
  • activeTabIconTint
  • tabsTintColor (apparently applies to title and icons)
  • tintColor
  • unselectedItemTintColor (doesn't specify if this applies to title and/or icons, probably the inverse of activeTabIconTint)
Tab
  • tintColor
  • activeTitleColor
  • titleColor

I added tintColor and activeTintColor to Tab as this is where the tab title colors are also defined. But there are a number of properties that can cause confusion due to poor naming and conflict with other properties. There's also a number of iOS specific properties that are exposed that could have cross-platform support with Android.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to mark this as a WIP and will clean up parity with iOS.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks Gary! Thats really worth it and a big improvement. And sorry for noising here 😄

@garymathews garymathews changed the title fix(android): amend TabGroup titleColor fix(android/ios): amend Tab tintColor and activeTintColor Apr 13, 2020
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

Requesting 2 changes:

  • Remove the "Could not update tab icon tint color..." warning message.
  • Setting "shiftMode" to false should show all tab titles.

Copy link
Contributor

@vijaysingh-axway vijaysingh-axway left a comment

Choose a reason for hiding this comment

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

iOS changes looks good.

@jquick-axway
Copy link
Contributor

jquick-axway commented Jun 5, 2020

@garymathews, with your newest changes the bottom tab titles never appear. I've tried setting "shiftMode" to false, true, 0, etc. None of these settings have an affect anymore.

function createTab(title) {
	var window = Ti.UI.createWindow({ title: title });
	window.add(Ti.UI.createLabel({ text: title + " View" }));
	var tab = Ti.UI.createTab({
//		icon: "/assets/images/tab1.png",
		title: title,
		window: window,
	});
	return tab;
}

var tabGroupSettings = {
	tabs: [createTab("Tab 1"), createTab("Tab 2"), createTab("Tab 3")],
//	tabsBackgroundColor: "blue",
//	tabsBackgroundSelectedColor: "blue",
};
if (Ti.Android) {
	tabGroupSettings.shiftMode = false;  // <- Not Working
	tabGroupSettings.style = Ti.UI.Android.TABS_STYLE_BOTTOM_NAVIGATION;
}
var tabGroup = Ti.UI.createTabGroup(tabGroupSettings);
tabGroup.open();

Edit: The tab titles were missing because the "shiftMode" switch block was missing break statements.

@tidev tidev deleted a comment from build Jun 5, 2020
Copy link
Contributor

@jquick-axway jquick-axway left a comment

Choose a reason for hiding this comment

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

CR: Pass

Copy link
Contributor

@ssjsamir ssjsamir left a comment

Choose a reason for hiding this comment

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

FR Passed: Tested with the test case above, able to see the tint colours on titles and icons.

Test Environment

MacOS Catalina: 10.15.5 Beta
Xcode: 11.4
Java Version: 1.8.0_131
Android NDK: 21.1.6273396-beta2
Node.js: 10.16.3
""NPM":"5.0.0","CLI":"8.0.0""
Andoird Pixl xl 7.1.1 Emulator 
iphone 8 13.4 sim

@build
Copy link
Contributor

build commented Jun 8, 2020

Fails
🚫 Tests have failed, see below for more information.
Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖 ❌ 1 tests have failed There are 1 tests failing and 702 skipped out of 7357 total tests.

Tests:

ClassnameNameTimeError
android.emulator.Titanium.UI.Android.ProgressIndicatordialog indeterminant - show in different windows (9)5.089
Error: timeout of 5000ms exceeded
at Titanium.<anonymous> (/ti-mocha.js:6535:53723)

Generated by 🚫 dangerJS against 9ab6b60

@sgtcoolguy
Copy link
Contributor

Manually merged to master. There was a small conflict preventing my merge/rebase, so had to do it locally.

@sgtcoolguy sgtcoolguy closed this Jun 8, 2020
@energy2080
Copy link

energy2080 commented Sep 11, 2020

This issue is back in latest 9.1.0.v20200824195336. Active tab is not highlighted

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

8 participants