Skip to content

Solution: Erick Noiztbander #10

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 9 commits into
base: main
Choose a base branch
from

Conversation

Noiztbander
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.

Buen proyecto Erick! Felicidades! Faltan muchos requisitos principales pero lo conseguido está bastante bien 👏🏻. Seguimos!

Comment on lines +17 to +18
// src="https://images.pexels.com/photos/1232594/pexels-photo-1232594.jpeg?auto=compress&cs=tinysrgb&dpr=3&h=750&w=1260"
// src="https://images.pexels.com/photos/6439051/pexels-photo-6439051.jpeg?auto=compress&cs=tinysrgb&h=750&w=1260"
Copy link
Contributor

Choose a reason for hiding this comment

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

En este caso como es un div se debería quitar el código comentado en la versión de entrega del proyecto,

@@ -0,0 +1,55 @@
import { createGlobalStyle } from "styled-components";
Copy link
Contributor

Choose a reason for hiding this comment

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

Es muy interesante la solución al cambio de light a dark mode pero en este caso sería más eficiente hacerlo con css normal sin importar la librería de Styled Components. Normalmente se usa una de las 2 cosas, o todo con SCSS o todo CSS in JS como Styled Components. Pero muy bien por tu parte por implementarlo y que funcione 👏🏻


import "./Footer.scss";

class Footer extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

En este caso podría ser una función normal dado que no se utiliza el state.

inputText: event.target.value,
});

// console.log(inputText);
Copy link
Contributor

Choose a reason for hiding this comment

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

Se debería eliminar todo el código comentado en la versión final de la aplicación.

placeholder="New Todo"
value={inputText}
onChange={this.formManagement}
className="font-bold font-big full-width input-no-border"
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 uso de utility classes para conseguir el input. Te recomiendo echarle un vistazo a tailwind 👀.

/>
</label>
<span className="font-bold font-big full-width">{toDo}</span>
<div className={`d-flex ${divHidden}`}>
Copy link
Contributor

Choose a reason for hiding this comment

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

El efecto de mostrar o no los botones se podrían conseguir con solo CSS lo cual evitaría cambiar el estado de la aplicación por cada hover que hagamos.


const LOCAL_STORAGE_KEY = "toDoListSaved";

function loadLocalStorageData() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Una forma de optimizar esta función sería pasar el nombre de local storage como parámetro de la función. De esta forma podemos evitar usar variables en el estado global.

Copy link
Contributor

Choose a reason for hiding this comment

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

Algo así:

function loadLocalStorageData(storageKey) {
  const prevToDos = localStorage.getItem(storageKey);

  if (!prevToDos) {
    return null;
  }

  try {
    return JSON.parse(prevToDos);
  } catch (error) {
    return null;
  }
}

selectedToDoToDelete(id) {
const { toDoList } = this.state;

const filteredToDoList = toDoList.filter((e, index) => index !== id);
Copy link
Contributor

Choose a reason for hiding this comment

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

No es muy recomendable usar el segundo parámetro index de los métodos de array como [].map o [].filter dado que no podemos asegurar que siempre sea el mismo index que tiene nuestro elemento. Mejor sería usar una librería como uuid que permite crear ids de forma segura.

updateToDo(id, toDo) {
const { toDoList } = this.state;

const updatedList = toDoList.map((e, index) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Los nombres de variable como e deberían ser una palabra completa como element para indicar de forma más clara para qué se usa la variable.

Comment on lines +108 to +117
// noActiveToDos() {
// // eslint-disable-next-line no-unused-vars
// const { toDoList } = this.state;
// toDoList.toDoList.filter(
// (e, index) => {isDone !== false}
// );
// this.setState({
// toDoList: filteredToDoList,
// });
// }

Choose a reason for hiding this comment

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

Commented code should not be pushed to GitHub.

}

editToDo() {
// const { editMode } = this.state;

Choose a reason for hiding this comment

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

Commented code should not be pushed to GitHub.

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