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

Move Toolbar to Ti.UI.Toolbar namespace #841

Merged
merged 4 commits into from
Aug 29, 2017
Merged

Conversation

ypbnv
Copy link
Contributor

@ypbnv ypbnv commented Aug 9, 2017

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

Description:
Remove implicit iOS namespace for Toolbar.
Add Android alloy parser for Toolbar.

I believe this would keep the backward compatibility for iOS since the new code is used only when the Toolbar is used as a custom ActionBar (which we will alloy only for Android). If we can further guard for that we could add that as well.

Add Android alloy parser for Toolbar.
@feons
Copy link
Contributor

feons commented Aug 16, 2017

@ypbnv, please also update samples and codes that reference Ti.UI.iOS.Toolbar, also Ti.UI.iOS.Toolbar.js should be removed.

@hansemannn
Copy link
Contributor

@feons Can we keep it until 8.0.0? Otherwise it will be a breaking change for Alloy, although we still support the tag. It's deprecated in 6.2.0 and will be removed in 8.0.0.

@hansemannn
Copy link
Contributor

@ypbnv Testing today. Does Alloy throw a deprecation-warning as well? Not sure if it's required though, since the SDK throws warnings already.

@ypbnv
Copy link
Contributor Author

ypbnv commented Aug 24, 2017

@hansemannn I don't know. I didn't even think about deprecation warnings, because it is something new for Android.

@hansemannn
Copy link
Contributor

I mean the Ti.UI.iOS namespace, but that's fine then. Will check it out!

@ypbnv
Copy link
Contributor Author

ypbnv commented Aug 24, 2017

Can I even access anything in that namespace? I have been building only for Android.

@feons
Copy link
Contributor

feons commented Aug 24, 2017

@hansemannn, sure. Actually I thought this is supposed to be a backward compatible change?

@feons feons closed this Aug 24, 2017
@feons feons reopened this Aug 24, 2017
@feons
Copy link
Contributor

feons commented Aug 24, 2017

ops.. closed by accident ...

Copy link
Contributor

@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.

CR / FT passed. Left some feedback about the general toolbar-implementation (not auto-filling by default), but this is Alloy-unrelated and could be addressed in a later release or 6.2.0.GA.

@hansemannn hansemannn merged commit 7251d04 into tidev:master Aug 29, 2017
@hansemannn
Copy link
Contributor

@feons Is the fix-version okay as it is?

@feons
Copy link
Contributor

feons commented Aug 29, 2017

@hansemannn,

  • could you please create an alloy ticket for this?
  • what fix-version are you referring to?
  • most importantly, there are quite a number of references to Ti.UI.iOS.Toolbar in the code base that are not taken care of, i.e sample code with Ti.UI.iOS.Toolbar usage does cause confusion; this probably won't work <Toolbar><Items> ... </Items></Toolbar> anymore.

@hansemannn
Copy link
Contributor

@feons Sorry for merging too early then! Should I create a ticket? The old toolbar still works for me, but please do another test to verify, that'd be awesome!

@feons
Copy link
Contributor

feons commented Aug 30, 2017

https://jira.appcelerator.org/browse/ALOY-1578 to track updates.

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

4 participants