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

Conversation

minicatsCB
Copy link
Owner

🔥 🔥 🔥 Prioritario

@UlisesGascon
Copy link
Collaborator

mñn por la mñn me pongo con ello. Te aviso en cuanto lo tenga

@UlisesGascon UlisesGascon self-requested a review March 24, 2019 08:54
Copy link
Collaborator

@UlisesGascon UlisesGascon left a comment

Choose a reason for hiding this comment

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

Feedback general!

  • Mola mucho que hayas incluido los Mocks!
  • Espero tu charla de Phaser en OSW :liada:
  • Creo que has forzado un poco la naturaleza de Firebase al meter un enfoque POO tan puro, pero la implementación de tu idea es muy correcta. Es más un tema de arquitectura en lo que podrías enfocarte para evolucionar tu proyecto.
  • Sería bueno incluir las reglas de seguridad en tu propio repo. Asé tienes el control de versiones también extendido a esa parte.
  • Sería bueno mejorar el contraste en la parte de la puntuación durante el juego.
  • Me sigue pareciendo un proyecto muy creativo!
  • En una versión mejorada me gustaría que cambiaran el orden de las palabras y que fueran un poco más random.
  • Sacaría un fichero de configuración con los parámetros del juego (tipo letra, fondos, etc..) para que se pudiera customizar mejor
  • Es un gran proyecto y muestra claramente lo mucho que has crecido técnicamente en estos meses 👏👏👏
  • Un ejercicio interesante sería replantearte el proyecto con un enfoque distinto con la arquitectura. Sería muy productivo explorar la idea detrás de los "estados de aplicaciones" en JavaScript... algo como Redux podría ayudarte con esto.
import { createStore } from 'redux';

/**
 * Esto es un reducer, una función pura con el formato
 * (state, action) => newState. Describe como una acción transforma el estado
 * en el nuevo estado.
 *
 * La forma del estado depende de tí: puede ser un primitivo, un array, un
 * objeto, o incluso una estructura de datos de Immutable.js. La única parte
 * importante es que no debes modificar el objeto del estado, en vez de eso
 * devolver una nuevo objeto si el estado cambió.
 *
 * En este ejemplo, usamos `switch` y strings, pero puedes usar cualquier forma
 * que desees si tiene sentido para tu proyecto.
 */
function counter(state = 0, action) {
  switch (action.type) {
  case 'INCREMENT':
    return state + 1;
  case 'DECREMENT':
    return state - 1;
  default:
    return state;
  }
}

// Creamos un store de Redux almacenando el estado de la aplicación.
// Su API es { subscribe, dispatch, getState }.
let store = createStore(counter);

// Puedes suscribirte manualmente a los cambios, o conectar tu vista
// directamente
store.subscribe(() => {
  console.log(store.getState())
});

// La única forma de modificar el estado interno es despachando acciones.
// Las acciones pueden ser serializadas, registradas o almacenadas luego para
// volver a ejecutarlas.
store.dispatch({ type: 'INCREMENT' });
// 1
store.dispatch({ type: 'INCREMENT' });
// 2
store.dispatch({ type: 'DECREMENT' });
// 1

@@ -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 ;-)

@@ -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

"public": "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! 💪


## 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 ;-)

review_please/package.json Show resolved Hide resolved
create() {
let canvarXMiddle = this.game.canvas.width / 2;
let titleText = this.add.text(canvarXMiddle, this.game.canvas.height / 4, 'Flash Typer', {
fontSize: '32px',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Todos estos valores serían más efectivos en un fichero de configuración facilmente modificables

}

update() {
// ÑapaScript®
Copy link
Collaborator

Choose a reason for hiding this comment

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

Efectivamente! Pero es muy respetable tu solución actual


createTextBox(scene, x, y) {
let textBox = scene.rexUI.add.textBox({
x: x,
Copy link
Collaborator

Choose a reason for hiding this comment

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

puedes dejarlo como x, y... no necesitas {x:x, y:y}

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

@@ -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 ^^

@minicatsCB
Copy link
Owner Author

Muchas gracias por la rápida review! 🙏

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.

2 participants