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-26068] Android: Improve Ti.UI.Toolbar default behavior #9367

Merged
merged 16 commits into from May 23, 2018

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Aug 29, 2017

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

Description:
Set a default Ti.UI.FILL width for the toolbar.
Center content.

Test case:

var window = Ti.UI.createWindow();

var toolbar = Ti.UI.createToolbar({
    bottom: 0,
    backgroundColor: 'red'
});

var items = [];

for (var i=0; i < 3; i++) {
    var button = Ti.UI.createButton({
        title: 'Button',
    });
    items.push(button);
}

toolbar.setItems(items);
window.add(toolbar);
window.open();

toolbar.addView(convertLayoutParamsForView(proxies[i].getOrCreateView()));
View tempView = convertLayoutParamsForView(proxies[i].getOrCreateView());
Toolbar.LayoutParams lp = new Toolbar.LayoutParams(ViewGroup.LayoutParams.WRAP_CONTENT, ViewGroup.LayoutParams.WRAP_CONTENT);
lp.gravity = Gravity.CENTER;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to expose this as a property in the future.

Copy link
Collaborator

@hansemannn hansemannn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, my tests should pass now. Approved!

P.S.: @garymathews may now if there is a more sexy way of setting the default-width to fill, but this should work.

@@ -63,6 +66,7 @@
public TiToolbar(TiViewProxy proxy) {
super(proxy);
toolbar = new Toolbar(proxy.getActivity());
toolbar.setContentInsetsAbsolute(0,0);
Copy link
Contributor

Choose a reason for hiding this comment

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

space after comma

@ypbnv
Copy link
Contributor Author

ypbnv commented Aug 29, 2017

@garymathews, @jquick-axway We have been talking with @hansemannn whether we should by default have the children be centered in the Toolbar or start from the beginning. As far as I tested Toolbar does not respect it's children's position attributes and acts solely as a container on native Android as well. So the proposition to have a property for begin\center gravity makes sense to me. And for further customization we can have setCustomView implemented.
What do you think about that?

@jquick-axway
Copy link
Contributor

@ypbnv, regarding the toolbar's child view layout positions/attributes being ignored, try this. Add our Java TiCompositeLayout as a view to the Toolbar with "autoFillsWidth" set to true and with a "horizontal" layout. And instead of adding child Titanium views to the Toolbar directly, add them to this TiCompositeLayout view which will serve as the container. I say this because our TiCompositeLayout is needed to handle our custom position/layout/padding settings in the child views. If you look at our TiCompositeLayout.onMeasure() method, then you'll see what I'm talking about.
https://github.com/appcelerator/titanium_mobile/blob/master/android/titanium/src/java/org/appcelerator/titanium/view/TiCompositeLayout.java

The only down side here is that our horizontal TiCompositeLayout view does not support right-to-left locales (ie: Arabic) versus I'm sure Android's native Toolbar would properly reverse the buttons/views.

@hansemannn
Copy link
Collaborator

Please note that iOS does not respect the margins / general styles as well, so Android probably should do the same there. It's simply because we want to follow the native behavior there - if people want to size the child-items, they would rather go with a manual horizontal layout which lays out the child-views in line. For now, I'd only change the width to Ti.UI.FILL by default and leave the native behavior as it is. But as @ypbnv said, maybe you as native Android devs see this different, I can only speak as a Titanium developer / end-user who would be fine and happy seeing the native behavior.

@ypbnv
Copy link
Contributor Author

ypbnv commented Aug 30, 2017

@jquick-axway That will do the trick, yes. And it is the reason I thought about setCustomView() implementation. With it we will have the option to use any view (TiCompositeLayout) and get advantage of it, but not forcing it whenever it is not actually needed.

Clean whitespaces from docs.
@hansemannn hansemannn changed the title [TIMOB-17964] Android: Improve default behavior. [TIMOB-17964] Android: Improve Ti.UI.Toolbar default behavior Feb 13, 2018
@jquick-axway
Copy link
Contributor

jquick-axway commented Feb 13, 2018

@ypbnv, Android typically left-aligns the Toolbar's content, right? And likely right-aligns for Arabic locales. I don't know if centering it is a good idea since that's not the native behavior.
Edit: Sorry, I just noticed that you removed the CENTER gravity setting. d'oh

Now, defaulting the Toolbar's width to Ti.UI.FILL does make sense to me. We do the same for other views such as TableView, ListView, ScrollView, etc. We don't document that they're filled by default, but I think most devs expect this to be the default behavior anyways.

@ypbnv ypbnv modified the milestones: 7.1.0, 7.2.0 Feb 20, 2018
@build
Copy link
Contributor

build commented Feb 21, 2018

Messages
📖

💾 Here's the generated SDK zipfile.

Generated by 🚫 dangerJS

@sgtcoolguy sgtcoolguy modified the milestones: 7.2.0, 7.3.0 May 16, 2018
@sgtcoolguy sgtcoolguy merged commit 07fd7dc into tidev:master May 23, 2018
@ypbnv
Copy link
Contributor Author

ypbnv commented May 25, 2018

I have updated the linked ticket.

@ypbnv ypbnv changed the title [TIMOB-17964] Android: Improve Ti.UI.Toolbar default behavior [TIMOB-26068] Android: Improve Ti.UI.Toolbar default behavior May 25, 2018
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

7 participants