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

PR Xel #1

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

PR Xel #1

wants to merge 23 commits into from

Conversation

vanyaxel
Copy link

No description provided.

@reloadercf
Copy link
Collaborator

@merunga @lupomontero @AdrianaHY este proyecto esta terminado tal vez quieran darle un vistazo al código y compartir algo de feedback adicional comparto el link de la implementación: https://conquering-planets.netlify.app/

Copy link
Collaborator

@reloadercf reloadercf left a comment

Choose a reason for hiding this comment

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

Algo que me gusto mucho fue el diseño. Tu código es fácil de leer, La primer recomendación que tengo es sobre la creación de mucho archivos .css , una alternativa puede ser el uso de css modules, una segunda recomendación es incorporar eslint para buenenas practicas, me gusto el uso de Redux muy Pro.

Comment on lines +1 to +12
import React from "react";
import { BrowserRouter, Switch, Route } from "react-router-dom";

import configureStore from './store/configureStore';
import { Provider } from 'react-redux';

import HomeView from './components/homeView/HomeView';
import Instructions from './components/instructions/Instructions';
import GeneralInfo from './components/General-Info/general-info/GeneralInfo';
import GameView from "./components/GameView/GameView";

import './App.css';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Me gusta el formato

Comment on lines +22 to +27
<Switch>
<Route exact path="/" render={() => <HomeView />} />
<Route path="/instructions" render={() => <Instructions />} />
<Route path="/general-info" render={() => <GeneralInfo />} />
<Route path='/game' render={() => <GameView />} />
</Switch>
Copy link
Collaborator

Choose a reason for hiding this comment

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

para Modularizar aun más puedes trasladar tus rutas a otro archivo

import React from 'react';
import ovni4 from '../../images/ovni4.png';
import Btn from '../button/Button';
import './game-over.css';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
import './game-over.css';
import './gameover.css';

<div className='game-over-view'>
<h1 className='text-game-over'>JUEGO TERMINADO</h1>
<h2 className='text-game-over-2'>!Felicidades haz ganado!</h2>
<img src={ovni4} alt="end" className='ovni4' />
Copy link
Collaborator

Choose a reason for hiding this comment

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

Recuerda que los buscadores usan la descripción para indexar imágenes

<h1 className='text-game-over'>JUEGO TERMINADO</h1>
<h2 className='text-game-over-2'>!Felicidades haz ganado!</h2>
<img src={ovni4} alt="end" className='ovni4' />
<Btn name="volver a jugar" route='/general-info' classLink='link-instructions' classBtn='btn-instruction' />
Copy link
Collaborator

Choose a reason for hiding this comment

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

me gusta que reutilizas los componentes

</div>
<div className='container-place-planets'>
<div className='text-place-planet planets'>
<p className='text-general-info'>Acomoda tus planetas en el tablero</p>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
<p className='text-general-info'>Acomoda tus planetas en el tablero</p>
<p className='text-general-info'>
Acomoda tus planetas en el tablero
</p>

Comment on lines +5 to +8
function Planet(props) {
return (
<>
<div className={props.classBtn}></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

destructuración

Suggested change
function Planet(props) {
return (
<>
<div className={props.classBtn}></div>
function Planet({classBtn}) {
return (
<>
<div className={classBtn}></div>

@@ -0,0 +1,100 @@
/* .container-instruction-view{
Copy link
Collaborator

Choose a reason for hiding this comment

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

si no se usa borralo

@@ -0,0 +1,34 @@
const columns = ['A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J'];
const rows = ['A', 1, 2, 3, 4, 5, 6, 7, 8, 9, 10];

Copy link
Collaborator

Choose a reason for hiding this comment

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

te recomiendo definir mejor el nombre del archivo

guessLocation: false
});
} else {
console.log('ya no puedes guardar mas ubicaciones');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log('ya no puedes guardar mas ubicaciones');

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