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 Dialog to ES6 #437

Closed
samreid opened this issue Jan 9, 2019 · 4 comments
Closed

Move Dialog to ES6 #437

samreid opened this issue Jan 9, 2019 · 4 comments

Comments

@samreid
Copy link
Member

samreid commented Jan 9, 2019

As a prelude to further work in #369, I'd like to see Dialog in ES6. I'll do that as a preliminary step.

UPDATE: There are 15 occurrences of Dialog.call( this, ... ), so we won't be able to use class until those cases are changed as well. According to the conversation in phetsims/phet-info#89 it is acceptable to port everything to ES6 except for the inherit part.

@samreid
Copy link
Member Author

samreid commented Jan 9, 2019

Not sure what else can be done aside from class/extends. @zepumph can you please take a look?

@zepumph
Copy link
Member

zepumph commented Jan 9, 2019

  • One potential improvement would be to make the internal type a class, but that may be confusing.

  • I also prefer curly braces for scope if the arrow function is more than a one-liner. What do you think about the above commit?

  • I removed function keyword from inherit call methods, how does that seem to you?

@zepumph zepumph assigned samreid and unassigned zepumph Jan 9, 2019
@samreid
Copy link
Member Author

samreid commented Mar 5, 2019

One potential improvement would be to make the internal type a class, but that may be confusing.

I think function CloseButton( options ) { should be deleted and replaced with usage of scenery-phet/js/buttons/CloseButton.js. If you agree, let's open a new issue for that.

I also prefer curly braces for scope if the arrow function is more than a one-liner. What do you think about the above commit?

No strong preference.

I removed function keyword from inherit call methods, how does that seem to you?

Looks great!

@zepumph
Copy link
Member

zepumph commented Mar 6, 2019

I created #486. Closing

@zepumph zepumph closed this as completed Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants