Solution: Jon Garcia y Sebastian Burpbacher #14
Conversation
preparing routes
This reverts commit c3a35e4.
| import Home from "./components/pages/Home"; | ||
| import Active from "./components/pages/Active"; | ||
| import Completed from "./components/pages/Complete"; | ||
| // import Todo from "./components/Todo"; |
There was a problem hiding this comment.
es recomendable eliminar todo el código comentado que no se utiliza
|
|
||
| handleDelete(itemId) { | ||
| const { todos } = this.state; | ||
| const notDelete = todos.filter((el) => el.id !== itemId); |
There was a problem hiding this comment.
Muy bien implementado!
Es recomendable nombrar las variables con un nombre descriptivo para que se entienda exactamente qué es la variable. En este caso como mejora se podría llamar el argument el como todo
| import React from "react"; | ||
| import "./_Button.scss"; | ||
| // import { Link } from "react-router-dom"; | ||
|
|
||
| const Checkbox = ({ ...newers }) => { | ||
| return <input className="check" type="checkbox" {...newers} />; | ||
| }; | ||
|
|
||
| function Button({ ...newers }) { | ||
| return <input className="but" type="button" {...newers} />; | ||
| } | ||
| function ClearBut({ ...newers }) { | ||
| return <input className="butClear" type="button" {...newers} />; | ||
| } | ||
| export { Button, Checkbox, ClearBut }; |
There was a problem hiding this comment.
Este fichero se debería extraer a varios componentes ya que el fichero se llama Button pero incluye componentes que no son botón como el checkbox.
| @@ -0,0 +1,24 @@ | |||
| import React from "react"; | |||
| import "./footer.scss"; | |||
There was a problem hiding this comment.
Es recomendable ordenar los imports para tener los imports de librerías primero y luego los imports de dependencias propias.
En este caso sería:
import React from "react";
import { Link } from "react-router-dom";
import "./footer.scss";
import { ClearBut } from "../Button/Button";| <Link to="/completed" className="nav_links"> | ||
| <h6>Completed</h6> | ||
| </Link> | ||
| <ClearBut onClick={clear} /> |
There was a problem hiding this comment.
Sería recomendable renombrar este componente a <ClearButton /> en vez de <ClearBut /> para ser más descriptivo.
| console.log(this.state); | ||
| } | ||
|
|
||
| handleEditing = (event) => { |
There was a problem hiding this comment.
Es recomendable definir las funciones de forma más consistente, o todas como arrow function o todas como método de la clase aunque funcione bien.
| @mixin grid3($cell1,$cell2,$cell3,$gap) { | ||
| display: grid; | ||
| grid-template-columns: $cell1 $cell2 $cell3; | ||
| column-gap: $gap; | ||
| justify-content: left; | ||
| align-content: flex-start; | ||
| } | ||
| @mixin resetMarginPadding { | ||
| margin: 0; | ||
| padding: 0; | ||
| } | ||
| @mixin inputs($color,$top,$bot){ | ||
| border-radius: 3px; | ||
| border: $borderColor; | ||
| height: 40px; | ||
| width: 80%; | ||
| justify-content: center; |
There was a problem hiding this comment.
Es recomendable dejar Prettier con la configuración de format on save de vscode para que el código esté siempre formateado y lo más legible posible.
| color: $color; | ||
| font-family: $fonts; | ||
| margin-bottom: $dist; | ||
| } |
There was a problem hiding this comment.
Como recomendación, siempre se debe dejar un salto de linea en los ficheros. Si usamos una herramienta como Prettier esto se hará de forma automática siempre.
| newTodo, | ||
| }) { | ||
| return ( | ||
| <> |
There was a problem hiding this comment.
Como main ya es un contenedor es mejor quitar el fragment ya que no hace falta.
| margin-right: 20px; | ||
| } | ||
|
|
||
| .but { |
There was a problem hiding this comment.
Los nombres de clases deberían ser más descriptivos para evitar palabras que puedan significar otra cosas en inglés como but. Un nombre más adecuado podría ser directamente .button
|
Os recomiendo repasar esta guia de cómo escribir buenos mensajes de commit ya que algunos del Pull Request no son muy óptimos o descriptivos. |
No description provided.