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

convert functions and variables to e6 syntax #352

Merged
merged 19 commits into from Aug 13, 2019

Conversation

@mayeedwin
Copy link
Contributor

commented Aug 7, 2019

functions to arrow functions

var to let

convert functions and variables to e6 syntax
`` functions to arrow functions ``

``var to let``
@jhuleatt

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

Hey @mayeedwin thanks for creating this!

There are a few things that need to be changed before we can merge your PR:

  • To fix the failing build, we need to tell eslint to use a newer version of ECMAScript (maybe es2017 to make sure it still works on older browsers?) by modifying the .eslintrc.json in the root of the quickstart-js repo. Eslint docs. I ended up doing this as part of #354
  • The indentation doesn't follow the convention of the rest of this repo and causes eslint to throw tons of errors. Maybe we'll switch to Prettier in the future, but for now please adjust the indentation to make eslint happy.
  • Let's use const instead of let where possible.

If you'd like me to handle any of these things, I'm happy to help. Let me know which ones you'd like to do! Once all the items are completed, I'll merge the PR :)

@mayeedwin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2019

Hey @mayeedwin thanks for creating this!

There are a few things that need to be changed before we can merge your PR:

  • To fix the failing build, we need to tell eslint to use a newer version of ECMAScript (maybe es2017 to make sure it still works on older browsers?) by modifying the .eslintrc.json in the root of the quickstart-js repo. Eslint docs.
  • The indentation doesn't follow the convention of the rest of this repo and causes eslint to throw tons of errors. Maybe we'll switch to Prettier in the future, but for now please adjust the indentation to make eslint happy.
  • Let's use const instead of let where possible.

If you'd like me to handle any of these things, I'm happy to help. Let me know which ones you'd like to do! Once all the items are completed, I'll merge the PR :)

Thanks for the heads up, sure, go ahead! E.g for the convention of the rest of this repo Will be jumping into it as well.

@jhuleatt

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2019

@mayeedwin I ended up changing the repo's .eslintrc in #354. If you sync with master, eslint should be happy with the more modern syntax in this PR. I've updated the checklist above.

mayeedwin added 9 commits Aug 9, 2019
@mayeedwin

This comment has been minimized.

Copy link
Contributor Author

commented Aug 10, 2019

@jhuleatt all good now?


var newAverage =
let newAverage =

This comment has been minimized.

Copy link
@jhuleatt

jhuleatt Aug 12, 2019

Contributor

please change this let to const since we never change the value of newAverage

This comment has been minimized.

Copy link
@mayeedwin

mayeedwin Aug 13, 2019

Author Contributor

Thanks for pointing that out. I over looked it. I'll need to fix it.

@jhuleatt

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2019

@mayeedwin thanks for the updates! A few more changes are needed. This article might help explain when to use let vs const.

mayeedwin added 9 commits Aug 13, 2019
@jhuleatt

This comment has been minimized.

Copy link
Contributor

commented Aug 13, 2019

Thanks @mayeedwin for your patience and for updating this!

@jhuleatt jhuleatt merged commit d23cb3b into firebase:master Aug 13, 2019

2 checks passed

cla/google All necessary CLAs are signed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.