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

Leverages ES6 in Notifications #5453

Merged
merged 2 commits into from Oct 27, 2017

Conversation

@VickyKoblinski
Copy link
Contributor

VickyKoblinski commented Oct 27, 2017

This task was mostly to familiarize myself with a piece of Riot.

  • Most changes are to variable declarations
  • Some functions changed to arrow functions
  • Fixed typo
@@ -15,33 +15,33 @@ limitations under the License.
*/

'use strict';
var React = require('react');
const React = require('react');

This comment has been minimized.

Copy link
@ara4n

ara4n Oct 27, 2017

Member

when updating to ES6, the correct way to fix these imports is actually to turn them into:

import React from 'react';

This technically works, but is very much not best practice.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 27, 2017

this is great - thanks! the only sticking point is to change the imports/requires to ES6-style for consistency with the rest of the update, but otherwise looks good to me. (Interestingly, it looks like we have a blind spot when doing ()=>{ return foo } rather than ()=>foo, so thanks for catching these!)

@VickyKoblinski

This comment has been minimized.

Copy link
Contributor Author

VickyKoblinski commented Oct 27, 2017

Sounds good to me! Changes have been made. ES6 is implicitly in strict mode, so I also removed 'use strict'.

@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 27, 2017

awesome! lgtm, thanks again :)

@ara4n ara4n changed the base branch from master to develop Oct 27, 2017
@ara4n

This comment has been minimized.

Copy link
Member

ara4n commented Oct 27, 2017

actually, there was one other catch: the PR is against master rather than develop, but I have changed the base in gihub successfully. technically you should also formally sign off the PR (see the DCO in contributing.rst) to confirm you are ok with the project licensing terms. going to merge anyway if you can put a DCO signoff in the comments here :)

@ara4n ara4n merged commit 3df1808 into vector-im:develop Oct 27, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@VickyKoblinski

This comment has been minimized.

Copy link
Contributor Author

VickyKoblinski commented Oct 28, 2017

Whoops, I'm sorry about that! I had read to do that, then it slipped me. I look forward to contributing more to such a great project! :)

Signed-off-by: Vicky Koblinski vickykoblinski@gmail.com

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.