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-25953] Android: Add "Ti.UI.TabbedBar" support #10286

Merged
merged 18 commits into from Jan 4, 2019

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Aug 24, 2018

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

Description:
Add TabbedBar implementation for Android.
Once this is ready for merging the idea is to remove the deprecation of Ti.UI.TabbedBar and move the the implementation of Ti.UI.iOS.TabbedBar back to the common namespace.
For now TabbedBar supports two different styles ( constants for styles may be renamed and moved to another place once we merge them with the styles currently supported in iOS ) which use two different native components on Android side:
TABBED_BAR_STYLE_DEFAULT uses an instance of TabLayout
TABBED_BAR_STYLE_BOTTOM_NAVIGATION_VIEW uses BottomNavigationView

Things I think are worth of addressing:

  1. Currently the style can't be changed after creation. I am not sure if this is possible on iOS, but if that is the case this needs to be implemented too. IMO keeping style as a creation-only property is fine.
  2. Different colors for different states (selected/unselected) can easily be supported for the selection indicator of TabLayout and the icons\text. Changing the background color of the whole tab will need a bit more "interesting" solution since they require an embedded in the .apk resource, thus making changing it dynamically a bit harder. Ideas about approaching this are welcome.
  3. BarItemType's properties supported on android are only title and image. width will need to be implemented either as a custom view or somehow translated through the gravity property. I think neither of those is a good solution, but if we want that there is room for experiment. enabled is clear how to be implemented, but I am curious what's the way of changing this property - through getting the BarItemType array from the component and changing it for the specific child there? I am not sure if this is the case in iOS.
  4. Do we want to support using custom views as tabs for Android at all?
  5. Android allows using text and icon at the same time. In this state the code uses icon instead of text if both are present ( as described in the docs for iOS ). Do we want to keep it that way for parity's sake ( I am OK with that ) or should we add the option for displaying both on Android?

Test case:
app.js

var win = Ti.UI.createWindow({ layout: 'vertical' });
var tabbedBar = Ti.UI.createTabbedBar({
    labels: ['One', 'Two', 'Three'],
    backgroundColor: '#336699',
    height: '10%',
    width: Ti.UI.FILL
});
var scrollView = Ti.UI.createScrollView({
    scrollingEnabled: false,
    height: '90%',
    scrollType: 'horizontal',
    layout: 'horizontal'
});
for (i = 0; i < tabbedBar.labels.length; i++) {
    scrollView.add( Ti.UI.createView({ backgroundColor: '#'+Math.random().toString(16).substr(-6)}))
}
tabbedBar.addEventListener('click', function(e) {
    scrollView.scrollTo(e.index * win.rect.width, 0, {animated: true});
})
win.add(scrollView);
win.add(tabbedBar);
win.open();

CC: @garymathews @hansemannn

On the way:
Expose the component through Alloy for Android.

@ypbnv ypbnv added this to the 7.5.0 milestone Aug 24, 2018
@build build added the docs label Aug 24, 2018
@jquick-axway
Copy link
Contributor

@ypbnv, great work!

I did some quick testing on iOS. I noticed that the style property is no longer supported by TabbedBar and ButtonBar. It's been deprecated since Titanium 3.4.2, but we haven't documented it as deprecated. Attempting to set the style property upon creation will log a warning message which can be seen here...
https://github.com/appcelerator/titanium_mobile/blob/master/iphone/Classes/TiUIButtonBar.m#L99

But this is good news because this gives us the freedom to change the behavior of this style property now. We can change the docs to state that it's creation-only, which I'm fine with. And perhaps the TabbedBar style constants should be moved to the Ti.UI module then? @hansemannn, what do you think?

Also, I've noticed that ButtonBar on iOS did not work like I expected. It doesn't display a checked/selected state. It's merely used as a row of buttons. This means that we should not treat it like a row of radio buttons like I originally suggested on Android and Windows. So, I'm thinking that we can have a radio button group style constant for TabbedBar instead. Perhaps not now, but in the future. I say this because an iOS UISegmentedControl is used like a radio button group too and not just for tab group buttons. What do you think?

For BarItemType, I'm okay with us only supporting properties title and image on Android. Although I think we need to support both text and image properties at the same time for the bottom bar style to meet the material design guideline. So, your addItem() method will need to support a value of type "String" and "HashMap".

One more thing. I've noticed that the Ti.UI.createTabbedBar() function does not exist on iOS. Yes, it's deprecated, but we still document it. So, watch out when writing a unit test for this or else it'll crash the app. I agree that we should un-deprecate it and deprecate the iOS version.

@hansemannn
Copy link
Collaborator

Hey @ypbnv, nice changes! I agree with everything @jquick-axway suggested, except:

  • If we want to support ButtonBar on Android, it should be done via the ButtonBar API, not via a style property. Simply for better usage in real world apps. We could internally just extend from a TabbedBar, give it a certain style and be ready to go.
  • I am a bit worries about the API design here. For me as a Ti-developer, I'd love to see the BottomNavigationView support, but rather via Ti.UI.TabGroup than Ti.UI.TabbedBar. The tabbed-bar always has been an inline UI-element on iOS and the BottomNavigationView with it's views, icons and titles is pretty much what iOS does with their UITabBar that is used for the tab-group. I know this may be a bigger change than expected, but having the bottom-navigation separated into the tab-group will be cleaner for the cross-platform design.

Finally, a tabbed-bar with views will be exactly what the Android tab-group is now, right? That may lead to confusion when trying to select proper API's. If we can conclude on those details, I see this being a huge step for parity!

@ypbnv
Copy link
Contributor Author

ypbnv commented Aug 27, 2018

@hansemannn , @jquick-axway
I don't mind moving the BottomNavigationView as a style for TabGroup if we agree to proceed like that. That may allow us to differentiate it from the TabLayout implementation a bit more. For instance we can allow both icon and title only for BottomNavigationView and leave the TabLayout as it is now - they are mutually exclusive. The only thing that bothers me - what happens if people want to use this UI component with Views instead of Windows?
As for ButtonBar - if it currently is only a container for buttons maybe we can introduce it the same way on Android, but with a support for a RADIO_GROUP_STYLE (or something like that) which would result in a radio group behavior. What do you think?

@hansemannn
Copy link
Collaborator

@ypbnv A style property on the tab-group for Android would make the most sense for this, yep! For the windows vs views case: People already use windows for tabgroups in a cross-platform fassion, so they will be happy to use windows for the bottom-navigation as well. And if they want to use (inline) tabs, they can stick with the (Android-only) views property of the tabbed-bar to achieve this.

@jquick-axway
Copy link
Contributor

I'd love to see the BottomNavigationView support, but rather via Ti.UI.TabGroup than Ti.UI.TabbedBar.

How about we do it for both? I do agree that TabGroup needs it too. TabbedBar is for those who want to do their own custom tab group handling/layouts. I like how @ypbnv did it with TabbedBar via the style property. I'm thinking the style constants supported by TabbedBar should also be supported by TabGroup too in the future.

Finally, a tabbed-bar with views will be exactly what the Android tab-group is now, right?

I think what @ypbnv meant was using a custom view within the tab button so that the developer can control the layout of the tab button's text, image, background, and other drawables. I think we should keep it simple for now, but it's definitely a good thought. :)

As for ButtonBar - if it currently is only a container for buttons maybe we can introduce it the same way on Android, but with a support for a RADIO_GROUP_STYLE (or something like that) which would result in a radio group behavior. What do you think?

I'm not sure what to do about ButtonBar at the moment. Perhaps it should simply be a horizontal layout of buttons on Android/Windows and that's it. If we do this, then perhaps it should be written in pure JavaScript as well. Maybe we should push this one out for now.

I definitely want us to support a radio button group in the future since that is a UI component that is missing in Titanium. Perhaps this should be a new Titanium view type such as Ti.UI.RadioButtonGroup or Ti.UI.OptionsBar (radio buttons are sometimes referred to as option buttons). This is a commonly used UI component on Android and Windows. On iOS, we would use a UISegmentedControl just like how it is used with TabbedBar. On Android, it would be nice to have a style property too where you can use the "chip" material theme (I'd like to fit this in somehow). Thoughts?

@hansemannn
Copy link
Collaborator

@jquick-axway Sounds great!

@jquick-axway
Copy link
Contributor

@hansemannn, @ypbnv, I've resurrected an old feature request ticket for radio button support. You can find it here...
https://jira.appcelerator.org/browse/TIMOB-3200

@ypbnv
Copy link
Contributor Author

ypbnv commented Sep 5, 2018

For now I am going for supporting BottomNavigation style in TabGroup. I think that will be better off as a separate ticket though. And here it is:
https://jira.appcelerator.org/browse/TIMOB-26354
I am leaving the style for TabbedBar, too. I will see if I can unify the style constants in a proper namespace. Also I will limit the unit test for Android only until we bring the namespace parity.

@hansemannn
Copy link
Collaborator

@ypbnv Sounds great! Cannot wait to use it in my personal projects.

@ypbnv ypbnv modified the milestones: 7.5.0, 8.0.0 Oct 18, 2018
@jquick-axway
Copy link
Contributor

@ypbnv, the TabGroup PR is merged now.

You probably want to use that PR's style constants and delete the constants under the TabbedBar proxy.

Remove the dedicated TabbedBar constants and instead use the defined in Android UI module.
@ypbnv
Copy link
Contributor Author

ypbnv commented Dec 14, 2018

@jquick-axway I replaced the constants. For other improvements (further customization) and under the hood code optimization I would prefer to have separate tickets. For the latter I have created one. I think when I am done with it I will have a better idea what customization may be shared between TabGroup and TabbedBar.

@build build requested review from a team December 14, 2018 16:40
@build
Copy link
Contributor

build commented Dec 14, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

📖

✅ All tests are passing
Nice one! All 2977 tests are passing.

Generated by 🚫 dangerJS against f69d186

@lokeshchdhry
Copy link
Contributor

FR Passed.

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.9-3
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)

type: Number
default: Titanium.UI.iOS.SystemButtonStyle.PLAIN
default: Titanium.UI.iOS.SystemButtonStyle.PLAIN for iOS, Ti.UI.TabbedBar.TABBED_BAR_STYLES_DEFAULT for Android
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to update the docs to reference the Ti.UI.Android.TABBED_BAR_STYLE_* constants.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the last change needed. @ypbnv, if you can do this by tomorrow, then that would be great. QE is ready to click that merge button. :)

@jquick-axway
Copy link
Contributor

jquick-axway commented Dec 17, 2018

We should un-deprecate Ti.UI.TabbedBar for iOS.
https://docs.appcelerator.com/platform/latest/#!/api/Titanium.UI.TabbedBar

Edit 1:
We can un-deprecate the iOS side after 8.0.0 RC. This really needs to be done by someone with a Mac (such as myself) since this involves changing our iOS code as well.

Edit 2:
I've written up a ticket to un-deprecate Ti.UI.TabbedBar on the iOS side below. I should have time to do this tomorrow.
https://jira.appcelerator.org/browse/TIMOB-26674

Use the common tabs style constants in the documentation.
Improve parity with iOS in case of TabbedBar items fallbacks when there is an image and title
properties.
@hansemannn
Copy link
Collaborator

hansemannn commented Dec 19, 2018

I have added ypbnv#1 for the iOS side. One critical parity issue that should be resolved before this slips into 8.0.0:

  - name: style
    summary: Style of the tabbed bar.
    description: |
        Specify one of the constants:
        For iOS: 
        [Titanium.UI.iOS.SystemButtonStyle](Titanium.UI.iOS.SystemButtonStyle),
        either `PLAIN`, `BORDERED`, or `BAR`.
        
        The `BAR` style specifies a more compact style and allows the bar's background
        color or gradient to show through.
        For Android:
        [Titanium.UI.TABS_STYLE_*]
        In Android [style](Ti.UI.TabbedBar.style) is only supported in the creation dictionary 
        of the proxy.
    type: Number
    default: Titanium.UI.iOS.SystemButtonStyle.PLAIN for iOS, Titanium.UI.TABS_STYLE_DEFAULT for Android

On iOS, the default style is Titanium.UI.iOS.SystemButtonStyle.PLAIN, on Android it is Titanium.UI.TABS_STYLE_DEFAULT. This could be moved to TABS_STYLE_DEFAULT all together. As this is an iOS-only change, I just need permission to do so.

EDIT: I did some research. The BAR style has been deprecated and removed a while ago already. The BORDERED one has been deprecated as of iOS 7 in favor of just using PLAIN. Soooo, I'd move PLAIN to TABS_STYLE_DEFAULT and clean up this mess. We're quite dependent on this new API, so we're happy to happy!

EDIT 2: Looks like TABS_STYLE_DEFAULT is currently on the UI.Android namespace. As iOS only has "PLAIN" left, I'll make the style property Android-only, as it doesn't have any effect on iOS anyways.

EDIT 3: Ready to review.

@jquick-axway
Copy link
Contributor

jquick-axway commented Dec 21, 2018

@hansemannn, I see what you mean. The TabbedBar "style" property is ignored and logs the following warning. (The same happens with ButtonBar.)

The style property has been deprecated in 3.4.2 and no longer has any effect

I tested it with the below code...

var window = Ti.UI.createWindow({
	fullscreen: true,
	layout: "vertical",
});
window.add(Ti.UI.createLabel({ text: "TabbedBar (PLAIN)", top: "10dp" }));
window.add(Ti.UI.iOS.createTabbedBar({
	style: Ti.UI.iOS.SystemButtonStyle.PLAIN,
	labels: ["One", "Two", "Three"],
}));
window.add(Ti.UI.createLabel({ text: "TabbedBar (BORDERED)", top: "10dp" }));
window.add(Ti.UI.iOS.createTabbedBar({
	style: Ti.UI.iOS.SystemButtonStyle.BORDERED,
	labels: ["One", "Two", "Three"],
}));
window.add(Ti.UI.createLabel({ text: "TabbedBar (BAR)", top: "10dp" }));
window.add(Ti.UI.iOS.createTabbedBar({
	style: Ti.UI.iOS.SystemButtonStyle.BAR,
	labels: ["One", "Two", "Three"],
}));
window.open();

tabbedbar-styles-ios

@jquick-axway
Copy link
Contributor

@ypbnv, @hansemannn, PR #10558 un-deprecates Ti.UI.TabbedBar for iOS and deprecates Ti.UI.iOS.TabbedBar instead.

@ypbnv, would you mind updating the API docs about this iOS change in this PR please? This way we avoid the merge conflict. Thanks.

Add deprecation version in docs for Ti.UI.iOS.TabbedBar.
@ypbnv
Copy link
Contributor Author

ypbnv commented Jan 3, 2019

@jquick-axway I added the deprecation version for the iOS namespace. Let me know if there is anything more to be updated.

Edit: I also changed the unit tests to be ran for both platform.

ypbnv and others added 2 commits January 3, 2019 14:18
Replaces the android-only tests with cross platform tests.
@lokeshchdhry
Copy link
Contributor

Re FR'ed & Ti.UI.TabbedBar seems to work as expected.

Studio Ver: 5.1.2.201812191831
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.9
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)

@lokeshchdhry lokeshchdhry merged commit 1123467 into tidev:master Jan 4, 2019
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

6 participants