-
Notifications
You must be signed in to change notification settings - Fork 170
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
Updated syntax to ES6 #1166
Updated syntax to ES6 #1166
Conversation
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.
Thanks for the change, Cam! It looks good, but I have some minor comments:
- let vs const: It's certainly a good practice not to reassign variables. We decided against replacing all
var
s withconst
as this would be in contrast to the common practice and look unfamiliar. Let's uselet
for "normal" variables andconst
for imports and constant values that are declared at the top of the file, e.g.
const FUNNY_REGEX = /^\s*(#.*)$/;
- We're currently releasing beta-2 and will merge your change after that, please use
2.0.0-beta2
as tabris dependency. We'll later batch-replace all dependencies to the next version. - Commit messages should use the imperative mood in the subject line. They should also indicate which part of the codebase is changed. Suggestion: "Update hello example to ES6".
Please see my other comments in your commit.
centerX: 0, top: 100, | ||
text: 'Show message' | ||
}).appendTo(tabris.ui.contentView); | ||
|
||
var textView = new tabris.TextView({ | ||
const textView = new TextView({ |
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.
Please use let
instead.
var button = new tabris.Button({ | ||
const {Button, TextView} = require('tabris'); | ||
|
||
const button = new Button({ | ||
centerX: 0, top: 100, | ||
text: 'Show message' | ||
}).appendTo(tabris.ui.contentView); |
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.
Avoid the global object tabris
. Import ui
and replace with appendTo(ui.contentView)
.
@@ -3,7 +3,10 @@ | |||
"description": "Tabris.js - Hello, World!", | |||
"category": "Small Apps", | |||
"main": "hello.js", | |||
"scripts": { | |||
"start": "http-server -c-1 -d false" |
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.
For 2.0, we recommend the CLI over starting a plain HTTP server: tabris serve
. This should make the npm script redundant.
"dependencies": { | ||
"tabris": "2.0.0-beta2" | ||
"tabris": "^2.0.0-beta1" |
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.
Please replace with 2.0.0-beta2
.
As a result of discussions with @ralfstx, I'm beginning a series of PR's on the examples to upgrade their syntax to ES6