Solution: Alvaro Merino and Miguel Perez #3
Conversation
| import * as api from "./api"; | ||
|
|
||
| // import * as api from "./api"; |
There was a problem hiding this comment.
Esta parte se debería eliminar y usar el import
| </section> | ||
| </main> | ||
| </Router> | ||
| </> |
There was a problem hiding this comment.
El Fragment se puede quitar ya que el Router es un container ya
| /> | ||
| </Route> | ||
| </Switch> | ||
| <footer className={darkMode ? "dark" : ""}> |
There was a problem hiding this comment.
El footer se podría extraer a un componente y renderizarlo en cada página.
| @@ -0,0 +1,30 @@ | |||
| @import url("https://fonts.googleapis.com/css2?family=Signika&display=swap"); | |||
|
|
|||
| .text__normal--check { | |||
There was a problem hiding this comment.
Muy buen uso de BEM! 👏🏻👏🏻
| align-items: center; | ||
| } | ||
|
|
||
| .container__hashtag > div > div { |
There was a problem hiding this comment.
Este selector es demasiado especifico. Una solución más optima es poner la clase directamente en el div final. La gran desventaja de este selector es que estamos ligando el CSS a la estructura del HTML y si queremos cambiar el HTML tendremos que cambiar el selector también.
| darkMode, | ||
| ...props | ||
| }) { | ||
| function isDisplayed() { |
There was a problem hiding this comment.
El nombre de la función debería cambiarse a algo como handleDisplayChange ya que isDisplayed representa normalmente una variable de tipo boolean que indica si está activo o no en este caso.
| changeHashtag={changeHashtag} | ||
| /> | ||
| </li> | ||
| </> |
There was a problem hiding this comment.
El Fragment se puede quitar ya que el li es un contenedor ya
| font-family: Arial, Helvetica, sans-serif; | ||
| } | ||
| html, | ||
| body, | ||
| #root, | ||
| .App { | ||
| width: 100%; | ||
| height: 100%; | ||
| } | ||
|
|
||
| body { | ||
| display: flex; | ||
| flex-direction: column; | ||
| } | ||
|
|
||
| main { |
There was a problem hiding this comment.
Los espacios entre bloques de CSS deberían ser más uniformes con una línea en blanco entre cada bloque.
| height: 20px; | ||
| border-radius: 4px; | ||
| margin: 0 0.5rem; | ||
| //align-self: stretch; |
There was a problem hiding this comment.
Commented code should not be pushed to GitHub.
|
|
||
| import * as api from "./api"; | ||
|
|
||
| // import * as api from "./api"; |
There was a problem hiding this comment.
Commented code should not be pushed to GitHub.
Solution for the Reactjs-todo-list exercise