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

Enable pull request for reviewing #1

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions review_please/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# flash-typer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Puedes incluir un poco más de info sobre el funcionamiento en el README ;-)

Flash Typer

## To play in production
Visit https://flash-typer.firebaseapp.com/.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Añadiría también una referencia a los mocks y el arte. Has invertido un buen tiempo en el diseño y deberías mostrar también esa parte de investigación ;-)

## To play in development
1. Clone the repo.
2. Do `npm install`.
3. Do `npx webpack`.
4. Do `npx firebase serve`.
10 changes: 10 additions & 0 deletions review_please/firebase.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"hosting": {
"public": "dist",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bien jugado ajustar la salida a dist

"ignore": [
"firebase.json",
"**/.*",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Buena programación defensiva! Evitando leaks! 💪

"**/node_modules/**"
]
}
}
Binary file added review_please/mockups/GameScreen.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added review_please/mockups/MainScreen.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added review_please/mockups/RankingScreen.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
29 changes: 29 additions & 0 deletions review_please/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
{
"name": "flash-typer",
"version": "1.0.0",
"description": "Flash Typer",
"main": "index.js",
"dependencies": {
minicatsCB marked this conversation as resolved.
Show resolved Hide resolved
"firebase-tools": "^6.4.0",
"phaser": "^3.16.2",
"phaser3-rex-plugins": "git+https://github.com/rexrainbow/phaser3-rex-notes.git"
},
"devDependencies": {
minicatsCB marked this conversation as resolved.
Show resolved Hide resolved
"file-loader": "^3.0.1",
"webpack": "^4.29.6",
"webpack-cli": "^3.2.3"
},
"scripts": {
"test": "echo \"Error: no test specified\" && exit 1"
Copy link
Collaborator

Choose a reason for hiding this comment

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

comando start missing. Deberías pasar algunos comandos del Readme.md a esta parte ;-)

},
"repository": {
"type": "git",
"url": "git+https://github.com/minicatsCB/flash-typer.git"
},
"author": "",
Copy link
Collaborator

Choose a reason for hiding this comment

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

No olvides añadirte! Aunque no vayas a subir la librería a NPM 😉

"license": "ISC",
Copy link
Collaborator

Choose a reason for hiding this comment

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

No estas usando licencia en el repo, pero si has dejado la licencia por defecto del generador

"bugs": {
"url": "https://github.com/minicatsCB/flash-typer/issues"
},
"homepage": "https://github.com/minicatsCB/flash-typer#readme"
}
28 changes: 28 additions & 0 deletions review_please/src/app.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import Phaser from "phaser";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Muy buena modularización en este punto. Has seguido las convenciones de Phaser!

import UIPlugin from 'phaser3-rex-plugins/templates/ui/ui-plugin.js';
import Main from "./scenes/main.js";
import Game from "./scenes/game.js";
import Ranking from "./scenes/ranking.js";

let config = {
type: Phaser.CANVAS,
width: 800,
heigth: 600,
physics: {
default: "arcade",
arcade: {
gravity: {y: 0},
debug: true
}
},
plugins: {
scene: [{
key: 'rexUI',
plugin: UIPlugin,
mapping: 'rexUI'
}]
},
scene: [Main, Game, Ranking]
};

let game = new Phaser.Game(config);
Binary file added review_please/src/assets/cartoon.jpg
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file added review_please/src/assets/paper.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
65 changes: 65 additions & 0 deletions review_please/src/gameService.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
class GameService {
constructor(){
let easyWords = [ "cat", "ghost", "cow", "bug", "snake", "lips", "socks", "coat", "heart", "kite", "milk", "skateboard", "apple", "mouse", "star", "whale", "hippo", "face", "spoon", "sun", "flower", "banana", "book", "light", "apple", "smile", "shoe", "hat", "dog", "duck", "bird", "person", "ball", "nose", "jacket", "beach", "cookie", "drum", "worm", "cup", "pie", "snowflake", "jar", "tree", "slide", "swing", "water", "ocean", "mouth", "eyes", "boy", "girl", "house", "bed", "shirt", "egg", "cheese", "circle"];
Copy link
Collaborator

Choose a reason for hiding this comment

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

Todo este array lo sacaría fuera o incluso en un JSON para que se pudiera cargar desde AJAX en la renderización del escenario. Pero podemos ver esto más adelante cuando veamos Backend y Express.js en el Master ^^

let mediumWords = [ "horse", "trip", "round", "park", "state", "baseball", "dominoes", "hockey", "whisk", "mattress", "circus", "cowboy", "skate", "thief", "spring", "toast", "half", "door", "backbone", "treasure", "pirate", "whistle", "coal", "photograph", "aircraft", "key", "frog", "pinwheel", "battery", "password", "electricity", "teapot", "nature", "outside", "spare", "platypus", "song", "bomb", "garbage", "ski", "palace", "queen", "computer", "lawnmower", "cake", "mailman", "bicycle", "lightsaber", "deep", "shallow", "America", "bowtie", "wax", "music"];
let difficultWords = [ "snag", "mime", "hail", "password", "newsletter", "dripping", "catalog", "laser", "myth", "hydrogen", "darkness", "vegetarian", "ditch", "neighborhood", "retail", "fabric", "jazz", "commercial", "double", "landscape", "jungle", "peasant", "clog", "bookend", "pharmacist", "ringleader", "diagonal", "dorsal", "macaroni", "yolk", "shrew", "wobble", "dizzy", "drawback", "mirror", "migrate", "dashboard", "download", "important", "bargain", "scream", "professor", "landscape", "husband", "comfy", "biscuit", "rubber", "exercise", "chestnut", "glitter", "fireside", "logo", "barber", "drought", "bargain", "professor", "vitamin"] ;

this.levels = {
easy: {
name: "easy",
posterVelocity: 50,
wordList: easyWords,
nextLevel: {
name: "medium",
neededScore: 30
},
isLastLevel: false
},
medium: {
name: "medium",
posterVelocity: 100,
wordList: mediumWords,
nextLevel: {
name: "difficult",
neededScore: 60
},
isLastLevel: false
},
difficult: {
name: "difficult",
posterVelocity: 200,
wordList: difficultWords,
isLastLevel: true
}
}

this.score = 0;
this.lives = 3;
}

getLevelByDifficulty(difficulty) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

sería interesante meter getters/setters puros

return this.levels[difficulty];
}

getNextLevel(currentLevel) {
return this.levels[currentLevel.nextLevel.name];
}

getCurrentScore() {
return this.score;
}

setScore(score) {
this.score = score;
}

getCurrentLives(){
return this.lives;
}

setLives(lives){
this.lives = lives;
}
}

export default GameService;
168 changes: 168 additions & 0 deletions review_please/src/login.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,168 @@
let instance = null;

class Login {
constructor(){
if (!instance) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Es un singleton camuflado. Me gusta ver que vas implementando patrones que vemos en clase 💪

Copy link
Owner Author

Choose a reason for hiding this comment

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

¿No es un poco peligroso poner una variable llamada instance (me parece un nombre con mucha probabilidad de existir por algún otro lado) en global que encima es tan importante para el Singleton? ¿Hay alguna forma mejor de implementarlo? Ahora mismo me parece que está como cogido con puntillas y que en cualquier momento va a fallar. 😆

this.loggedInUsername = "Not logged in";
this.isUserLoggedIn = false;
this.hasAuthStateChanged = false;

this.initFirebase();
this.setAuthStateObserver();

instance = this;
}

return instance;
}

get loggedInUsername() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Buen uso de Getters y Setters!

Copy link
Owner Author

Choose a reason for hiding this comment

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

Las variables loggedInUsername,isUserLoggedIn y hasAuthStateChanged me parecen un poco ñapa, en verdad. Tenía la idea de que solo hubiera una variable user con su getter y setter y listo. Pero entre el funcionamiento de Firebase y Phaser, cada uno por su lado, no he conseguido hacer algo más "compacto" sin tantas variables "sueltas". Sobre todo, ha sido el authStateObserver lo que más me ha limitado (ver ñapa aquí).

return this._loggedInUsername;
}

set loggedInUsername(username) {
this._loggedInUsername = username;
}

get isUserLoggedIn() {
return this._isUserLoggedIn;
}

set isUserLoggedIn(loginState) {
this._isUserLoggedIn = loginState;
}

get hasAuthStateChanged() {
return this._hasAuthStateChanged;
}

set hasAuthStateChanged(authState) {
this._hasAuthStateChanged = authState;
}

initFirebase() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Esta inicialización podrías haberla generado fuera de aquí, ya que es en sí la inicialización de la propia librería. Si alguien quiere clonarse el repo para lanzado en local, tiene que buscar mucho para poner su propio Firebase. Además este paso no esta explicado en el README.md

Copy link
Owner Author

Choose a reason for hiding this comment

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

¿Te refieres en el propio HTML?

var config = {
apiKey: "AIzaSyDhZLEfkH6ydtLOEBzjQKjCIjnIwtLooQs",
authDomain: "flash-typer.firebaseapp.com",
databaseURL: "https://flash-typer.firebaseio.com",
projectId: "flash-typer",
};

firebase.initializeApp(config);
}

setAuthStateObserver() {
firebase.auth().onAuthStateChanged(this.authStateObserver.bind(this));
}

authStateObserver(user) {
this.hasAuthStateChanged = true;
if (user) {
this.loggedInUsername = user.displayName|| "No name";
this.isUserLoggedIn = true;
console.log("User is signed in as:", this.loggedInUsername);
} else {
this.loggedInUsername = "Not logged in";
this.isUserLoggedIn = false;
console.log("User is signed out as:", this.loggedInUsername);
}
}

signInWithGithub() {
var provider = new firebase.auth.GithubAuthProvider();
firebase.auth().signInWithPopup(provider).then(result => {
console.log("User signed up with Github succesfully");
if (result.additionalUserInfo.isNewUser) {
let userData = {
email: result.user.email,
displayName: result.additionalUserInfo.profile.login,
photoURL: result.additionalUserInfo.profile.avatar_url
};
this.updateCurrentUserAuthProfile(userData);
this.saveUserDataInDatabase(userData);
}

this.loggedInUsername = result.additionalUserInfo.profile.login;
}).catch(function(error) {
console.log("An error ocurred while signing in with Github. Error:", error);
});
}

updateCurrentUserAuthProfile(userData) {
firebase.auth().currentUser.updateProfile({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Muy muy bien jugado para evitar tener que gestionar tu los nodos en la base de datos! 👏 👏 👏

email: userData.email,
displayName: userData.displayName,
photoURL: userData.photoURL
}).then(() => {
console.log("Profile update succesful");
}).catch(error => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sería interesante tener un gestor de errores en general para alertar al usuario también 😉

Copy link
Owner Author

Choose a reason for hiding this comment

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

Respecto a esto, tienes alguna sugerencia de cómo hacerlo? 🤔

console.log("An error ocurrer while updating user profile. Error:", error);
});
}

saveUserDataInDatabase(userData) {
firebase.database().ref().child('users').push(userData).then(() => {
console.log("User data saved in database succesfully");
}).catch(error => {
console.log("An error ocurred while saving user data in database. Error:", error);
});;

}

saveUserScoreInDatabase(achievedScore) {
let user = firebase.auth().currentUser;

if(user != null) {
return this.getUserByEmail(user.email).then(foundUser => {
if(foundUser) {
let update = {};
update["users/" + foundUser.key + "/achievedScore"] = achievedScore;
return firebase.database().ref().update(update).then(() => {
console.log("User score updated succesfully");
}).catch(error => {
console.log("An error ocurred while updating user score. Error:", error);
});
} else {
console.log("User not found in database. Can't save its score");
}
});
}
}

getUserByEmail(email) {
let user;
Copy link
Collaborator

Choose a reason for hiding this comment

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

realmente necesitas esta variable?

return firebase.database().ref().child("users")
.orderByChild("email")
.equalTo(email)
.once("value")
.then(snapshot => {
snapshot.forEach((childSnapshot) => {
user = childSnapshot;
});

return user;
}).catch(error => {
console.log("An error ocurred while searching user in database. Error:", error);
});
}

getUsersRanking() {
let users = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

puedes declararte esta variable en su scope directamente

return firebase.database().ref().child("users")
.orderByChild("achievedScore")
.once("value")
.then(snapshot => {
snapshot.forEach((childSnapshot) => {
users.push(childSnapshot.val());
});

return users.reverse(); // We want the users ordered in descending order
});
}

signOut() {
firebase.auth().signOut();
}
}

export default Login;
Loading