Skip to content

Solution: Brahim Benalia & Marc Solà Ramos #8

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

Open
wants to merge 42 commits into
base: main
Choose a base branch
from

Conversation

bbenalia
Copy link

No description provided.

@danilucaci danilucaci self-requested a review May 28, 2021 11:02
Copy link
Contributor

@danilucaci danilucaci left a comment

Choose a reason for hiding this comment

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

Muy buen proyecto! Felicidades 👏🏻! Las partes que se han cumplido están bastante bien

Comment on lines +53 to +58
if (!prevItems || !prevItems.todos.length) {
api.getProducts().then((data) => {
this.setState({ todos: data });
});
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy interesante el check para ver si el localstorage tiene todos o no.

const { todos } = this.state;

const newTodo = {
id: uuidv4(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy buen uso de uuid para crear ids 👏🏻

const { todos } = this.state;
const arr = todos.map((todo) => {
return todo.id === id ? { ...todo, complete: !todo.complete } : todo;
// return obj;
Copy link
Contributor

Choose a reason for hiding this comment

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

Este código comentado se debería eliminar ya que no se usa

handleEdit(id) {
const { todos } = this.state;
const todoToEdit = todos.map((todo) => {
return todo.id === id ? { ...todo, edit: true } : todo;
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy bueno uso del operador ternario para la comprobación 👏🏻

letter-spacing: 10px;
color: white;
}
.TODO__Header__DarkMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

Si se sigue BEM como nomenclatura esta clase debería como: TODO__Header--DarkMode

Block__Element--Modifier

Aquí puedes ver algunos ejemplos

Comment on lines +1 to +2
/* eslint-disable jsx-a11y/no-static-element-interactions */
/* eslint-disable jsx-a11y/click-events-have-key-events */
Copy link
Contributor

Choose a reason for hiding this comment

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

Estas reglas ya indican que este componente se debería haber hecho con un input y aplicando los estilos gráficos. Normalmente se aplica al label del checkbox para conseguir unos estilos propios y mantener la posibilidad de que siga siendo un elemento de formulario.

}}
hasErrorMessage={touched.name}
errorMessage={errors.name}
autoFocus={true}
Copy link
Contributor

Choose a reason for hiding this comment

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

Los siguientes props en React son equivalentes, consiguen el mismo resultado (indicar que el valor del prop es true), por tanto se recomienda usar la versión más corta:

// evitar
autoFocus={true}
// Ok
autoFocus

<div className="switchHandle" />
</div>
</button>
<span aria-label="" role="img">
Copy link
Contributor

Choose a reason for hiding this comment

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

El atributo aria-label se usa para indicar el nombre que usarán los lectores de pantalla. Un ejemplo válido para este botón sería: aria-label = "change theme"

Comment on lines +11 to +17
todo = {},
handleRemove = () => {},
handleChangeCheck = () => {},
handleEdit = () => {},
handleEditSubmit = () => {},
handleResetEdit,
currentTheme,
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy buen uso de los valores por defecto pero habría que añadirlos a todos los props.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants