-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: webpack template for classic apps #11858
Conversation
Tests:
|
@@ -0,0 +1,6 @@ | |||
<?xml version="1.0" encoding="utf-8"?> |
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.
Is this file something that may get generated/overridden by the semantic.colors.json
file for light/dark themes?
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.
AFAIK we generate a ti.semantic.colors.xml
for that, so we don't touch the default colors.xml
.
attributes: [ { | ||
type: Ti.UI.ATTRIBUTE_FOREGROUND_COLOR, | ||
value: '#E82C2A', | ||
range: [ 5, 7 ] |
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.
totally unrelated to this PR, but this syntax really confuses me for range. I guess since I'm used to mathematical ranges being declared like (0, 5]
or [1, 6]
or [1, 6)
where (
is exclusive and [
is inclusive. Obviously this is JS syntax and an array, but my brain immediately thinks: "oh, he's selecting text indices 5 to 7 inclusive" when actually you're selecting text starting at index 5, but for the next 7 characters. Not sure if anyone has any thoughts on making this more obvious without breaking backwards compatibility....
cc @jquick-axway @ewanharris @garymathews @vijaysingh-axway
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 can totally relate. While writing this i was like, what the hell why is this range not doing what i want? This does not make any sense! Until i eventually looked up the docs and found out that the second value is the length, and not the end of the range.
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 could modify the "range" property to accept either a dictionary or array. With a dictionary, you effectively have named parameters, making it more obvious how it works.
var attributedString = Ti.UI.createAttributedString({
text: "Hello World",
attributes: [{
type: Ti.UI.ATTRIBUTE_UNDERLINES_STYLE,
value: Ti.UI.ATTRIBUTE_UNDERLINE_STYLE_SINGLE,
range: { // Add support for dictionary.
index: 6,
length: 5,
},
}, {
type: Ti.UI.ATTRIBUTE_BACKGROUND_COLOR,
value: "red",
range: [6, 5], // <- The old way.
}],
});
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.
Just another +1 on "wtf does that syntax mean". I like Josh's suggestion, it makes it a lot clearer to me
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 dictionary seems like a great idea. I would prefer { start: 6, length: 5 }
though ;)
const username = Ti.UI.createLabel({ | ||
text: 'Verona Blair', | ||
font: { | ||
fontSize: 28 |
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.
Another nitpick unrelated to this PR: why do we name the property fontSize
if it's going to be inside a font
object. Seems redundant.
const fontFamily = fontMap[style]; | ||
return Ti.UI.createLabel(Object.assign({}, { | ||
font: { | ||
fontFamily, |
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.
same deal as above, feels very redundant to me to preface these properties with font
in the name if they'll be under a font object/property.
android/templates/app/webpack-default/template/platform/android/README.md
Outdated
Show resolved
Hide resolved
const title = 'Profile'; | ||
const window = createWindow({ title, layout: 'vertical' }); | ||
|
||
const scrollView = Ti.UI.createScrollView({ layout: 'vertical' }); |
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.
You need to set creation property scrollType
to "vertical"
or else a warning will be logged on Android. This is because Android's native ScrollView
does not support scrolling both vertically and horizontally like iOS. It needs to be told upon creation which direction to support. (It defaults to vertical, but will log a warning if the property is missing.)
const scrollView = Ti.UI.createScrollView({
layout: 'vertical',
scrollType: 'vertical'
});
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.
Would it make sense to have a default value of vertical
for that? Most of the time when i use scroll views i want them to be vertical anyway.
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.
It already defaults to "vertical" now, but it logs a warning stating that you should explicitly set it. This warning comes from our code and it's been this way for years.
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.
No comments from me, this is awesome @janvennemann huge kudos for going above and beyond here
@@ -0,0 +1,5 @@ | |||
{ | |||
"recommendations": [ | |||
"axway.vscode-titanium" |
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.
Love it
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 didn't know we can do this. We should do the same in all of our templates.
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.
@janvennemann, @ewanharris, maybe we should add a /.vscode/settings.json
file which tells the editor to ignore our generated directories. What do you think?
{
"files.exclude": {
"build/": true,
"i18n/": true,
"platform/": true,
"Resources/": true
},
"search.exclude": {
"build/": true,
"i18n/": true,
"platform/": true,
"Resources/": true
},
"files.watcherExclude": {
"build/": true,
"i18n/": true,
"platform/": true,
"Resources/": true
}
}
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 wrote up a ticket to add these vscode files to our other app templates here...
https://jira.appcelerator.org/browse/TIMOB-28070
FR Passed. Webpack template for classic apps looks good. This being the base for alloy & angular template, alloy template works as expected. Apps with this template build & run fine on Android & IOS on MAC & Windows. Angular template will be tested in this PR #11852 |
JIRA: https://jira.appcelerator.org/browse/TIMOB-27716
Optional Description:
This is a revamped version of the default two-tabbed app template for Webpack enabled projects. It adds a bunch of additional stuff that isn't in the original default template, but where i felt that users will probably add it anyway sooner or later.
appicon.png
, enabling rounded and adaptive icons. The launch image is a single 9-patch PNG file.DefaultIcons.png
in the project root, while the launch image comes fromLaunchLogo.png
.Note
The updated Angular template and this template both contain a very similar hook to do various post project creation tasks like installing NPM dependencies. I plan to move this hook out of the templates to make it reusable. I'll do this next week in one of these two new template PRs.This PR now contains the shared post-create hook for Webpack enabled project and needs to be merged before #11852 can be tested.