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): Fixes changing background colors for Tabs after they are drawn. #10756
Conversation
…e drawn. Fixes changing TabGroup and Tab background colors for different states after the creation of the components. Fixes TIMOB-26859.
|
@@ -48,6 +48,31 @@ public TiUITabLayoutTabGroup(TabGroupProxy proxy, TiBaseActivity activity) | |||
super(proxy, activity); | |||
} | |||
|
|||
private void setDrawablesForTab(int tabIndex) | |||
{ | |||
TabProxy tabProxy = ((TabGroupProxy) this.proxy).getTabList().get(tabIndex); |
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.
We need to validate the given tabIndex
before using it. This is especially needed if the TabGroup
has no tabs. I've seen some app devs add/remove tabs dynamically before.
ArrayList<TabProxy> tabProxiesList = ((TabGroupProxy) this.proxy).getTabList();
if ((tabIndex < 0) || (tabIndex >= tabProxiesList.length())) {
return;
}
TabProxy tabProxy = tabProxiesList.get(tabIndex);
if (tabProxy == null) {
return;
}
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.
We should test this with an empty TabGroup
just in case.
function createTab(title) {
var window = Ti.UI.createWindow({ title: title });
window.add(Ti.UI.createLabel({ text: title + " View" }));
return Ti.UI.createTab({ title: title, window: window });
}
var tabGroup = Ti.UI.createTabGroup({
// tabs: [createTab("Tab 1"), createTab("Tab 2"), createTab("Tab 3")],
// style: Ti.UI.Android.TABS_STYLE_BOTTOM_NAVIGATION,
tabsBackgroundColor: "red",
tabsBackgroundSelectedColor: "orange",
});
tabGroup.tabsBackgroundColor = "green";
tabGroup.tabsBackgroundSelectedColor = "yellow";
tabGroup.addEventListener("open", function() {
tabGroup.tabsBackgroundColor = "purple";
tabGroup.tabsBackgroundSelectedColor = "blue";
});
tabGroup.open();
@@ -356,6 +361,9 @@ public void propertyChanged(String key, Object oldValue, Object newValue, KrollP | |||
this.swipeable = TiConvert.toBoolean(newValue); | |||
} else if (key.equals(TiC.PROPERTY_SMOOTH_SCROLL_ON_TAB_CLICK)) { | |||
this.smoothScrollOnTabClick = TiConvert.toBoolean(newValue); | |||
} else if (key.equals(TiC.PROPERTY_TABS_BACKGROUND_COLOR) | |||
|| key.equals(TiC.PROPERTY_TABS_BACKGROUND_SELECTED_COLOR)) { | |||
setDrawables(); |
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.
The setDrawable()
call here will create views if currently null. So, we need to test if this works okay when setting these properties after the createTabGroup()
call and before the TabGroup.open()
call. This means the created tabs will be using the previous activity context instead of the tab group activity context.
Would you mind testing the following please?
function createTab(title) {
var window = Ti.UI.createWindow({ title: title });
window.add(Ti.UI.createLabel({ text: title + " View" }));
return Ti.UI.createTab({ title: title, window: window });
}
var tabGroup = Ti.UI.createTabGroup({
tabs: [createTab("Tab 1"), createTab("Tab 2"), createTab("Tab 3")],
// style: Ti.UI.Android.TABS_STYLE_BOTTOM_NAVIGATION,
});
tabGroup.tabsBackgroundColor = "blue"; // <- Will this work?
tabGroup.tabsBackgroundSelectedColor = "red"; // <- Will this work?
tabGroup.open();
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.
The use-case here is an app that wants to to dynamically change the color of a tab. For example, to highlight a tab when new content is available or a notification has been received. An Alloy user typically has the creation properties hard-coded in XML, so, the only option is to change it dynamically in the view code. They may do this before the open call.
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.
I updated the PR. I will open a backport to have it ready in case no further changes are done.
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.
CR: Pass
@ypbnv Please could you take a look at the merge conflicts? |
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.
FR Passed: Able to change background colors for Tabs after they are drawn. Tested using the test cases mentioned in the description above.
Test Environment
Pixel XL api 28
iPhone 6(12.1 Sim)
APPC CLI: 7.0.10-17
Operating System Name: Mac OS Mojave
Operating System Version: 10.14.2
Node.js Version: 8.9.1
Xcode 10.1
JIRA: https://jira.appcelerator.org/browse/TIMOB-26859
Description:
Fixes changing TabGroup and Tab background colors for different states after the creation of the
components.
Note: No unit tests. Also I see that in the code sample in the ticket the property
backgroundSelectedColor
(https://docs.appcelerator.com/platform/latest/#!/api/Titanium.UI.Tab-property-backgroundSelectedColor) is being used. When I was refactoring the TabGroup I went for the usage ofbackgroundFocusedColor
(https://docs.appcelerator.com/platform/latest/#!/api/Titanium.UI.Tab-property-backgroundFocusedColor) as it explicitly states that is used for changing the color of the Tab according to the state. Do you think we can adjust the docs to just list the latter and add to the description that it acts asbackgroundSelectedColor
?Test Case 1:
This tests empty tab group, top/bottom tab color handling, and setting colors after creation but before open.
// tabs: ...
line in the code below.// style: ...
line in the code below.Test Case 2:
This tests changing tab colors after opening tab group.