Skip to content

Conversation

@samtstern
Copy link
Contributor

No description provided.

@samtstern samtstern requested a review from abeisgoat February 13, 2018 22:48
@samtstern
Copy link
Contributor Author

@AbeHaskins PTAL. I think I got all the this --> that working right, or at least FriendlyEats appears to work fine for me.

for (let r = 0; r < 5*Math.random(); r++) {
const rating = this.data.ratings[
var ratingPromises = [];
for (var r = 0; r < 5*Math.random(); r++) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just noticed how amazing this is. The r < 5 * Math.random() part will execute on each loop so it's super, duper random.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty funny. I wonder what the distribution of actual r's is lol.

Copy link
Contributor

@abeisgoat abeisgoat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

for (let r = 0; r < 5*Math.random(); r++) {
const rating = this.data.ratings[
var ratingPromises = [];
for (var r = 0; r < 5*Math.random(); r++) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's pretty funny. I wonder what the distribution of actual r's is lol.

importScripts('/__/firebase/4.8.1/firebase-messaging.js');
importScripts('/__/firebase/init.js');

const messaging = firebase.messaging();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch here is debatable because FWIW any browser that supports service workers are also gonna support es6 syntax, but I guess it's fine for consistency.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. Maybe we revisit the ES6 decision but as it stands it's easier to just be consistent.

@abeisgoat
Copy link
Contributor

Ya'll got another PR brewing for friendlyeats-web so they stay consistent?

@samtstern
Copy link
Contributor Author

@AbeHaskins I'll get that friendlyeats PR going now, good call.

Copy link
Contributor

@nicolasgarnier nicolasgarnier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small nits, otherwise LGTM :)

"importScripts": true,
"FriendlyEats": true
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add missing End of file line break :)

"globals": {
"importScripts": true
}
} No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add end of file Line break :)

scripts/test.sh Outdated
set -e
# Run linter
find . -type f -name "*.js" -not -path "*node_modules*" \
| xargs eslint No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add end of file line break

.travis.yml Outdated
- npm install -g eslint
- lerna bootstrap
script:
- ./scripts/test.sh No newline at end of file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add end of file line break

*/
'use strict';

var componentHandler = componentHandler || undefined;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add componentHandler to the globals instead ?

@nicolasgarnier
Copy link
Contributor

LGTM! Thanks for doing this @samtstern !

@nicolasgarnier nicolasgarnier merged commit e698552 into master Mar 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants