Skip to content

Solution: Jordi Arnau & Eunyoung Kim #4

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

Conversation

solaz0824
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! 👏🏻👏🏻 Está muy bien logrado incluso con extras. Felicidades!


if (!data) {
return {
id: Math.random() * 1000,
Copy link
Contributor

Choose a reason for hiding this comment

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

Es más recomendable usar una librería como uuid para generar ids ya que son más seguros.

Comment on lines +8 to +38
body.dark {
background-color: $mainDark;
.btn {
path {
fill: $mainWhite;
}
}
.mainScreen {
.upperScreen {
background-image: linear-gradient(rgba(0, 0, 0, 0.5), rgba(0, 0, 0, 0.5)),
url("../src/img/background.jpg");
.addTodo {
form {
input {
background-color: $mainDark;
color: $mainWhite;
}
.addBtn {
background-color: #2b425b;
}
}
}
.TodoList {
background-color: rgb(3, 17, 12);
color: $mainWhite;
.footerStyle {
.links {
color: #ebd2e3;
}
.filtered {
color: $mainPurple;
Copy link
Contributor

Choose a reason for hiding this comment

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

Es mejor tener clases menos anidades para evitar que haya una especificidad muy elevada. Una forma muy simple de conseguirlo sería tener todas las clases al mismo nivel y no definidas de otras clases.

Copy link
Contributor

Choose a reason for hiding this comment

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

body.dark {
  background-color: $mainDark;
  .btn {
    path {
      fill: $mainWhite;
    }
  }
  .upperScreen {
    background-image: linear-gradient(rgba(0, 0, 0, 0.5), rgba(0, 0, 0, 0.5)),
      url("../src/img/background.jpg");
  }
  .addTodo {
    input {
      background-color: $mainDark;
      color: $mainWhite;
    }
    .addBtn {
      background-color: #2b425b;
    }
  }
  .TodoList {
    background-color: rgb(3, 17, 12);
    color: $mainWhite;
    .links {
      color: #ebd2e3;
    }
    .filtered {
      color: $mainPurple;
    }
    .footerBtn {
      color: $mainWhite;
    }
    .todo {
      border-bottom: 1px solid rgba(61, 1, 56, 0.445) !important;
      background: rgb(2, 0, 36);
      background: linear-gradient(
        90deg,
        rgba(2, 0, 36, 1) 0%,
        rgba(39, 29, 50, 1) 35%,
        $mainPurple 100%
      );
      color: $mainWhite;
      button {
        svg {
          fill: $mainWhite;
        }
      }
    }
  }
  .roundedLabel {
    border-color: rgba(255, 255, 255, 0.15);
  }
}

Comment on lines +18 to +26
<input
type="text"
className="form-control"
todo={todo}
placeholder="Add To Do"
onChange={handleChange}
value={todo}
required
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Los inputs deberían tener siempre un label asignado con un text que describa el input. Si no queremos que se vea visualmente el label podemos usar una convención muy común que usa clases llamadas .sr-only que lo que hace es esconder visualmente el elemento en la página pero si que está renderizado para los lectores de pantalla y el navegador.

.sr-only {
  position: absolute;
  width: 1px;
  height: 1px;
  padding: 0;
  margin: -1px;
  overflow: hidden;
  clip: rect(0, 0, 0, 0);
  border: 0;
}

Comment on lines +16 to +20
if (theme === lightTheme || theme === darkTheme) {
body.classList.add(theme);
} else {
body.classList.add(lightTheme);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Muy bien implementado el cambio de oscuro a claro. 👏🏻👏🏻

Comment on lines +3 to +25
.mainScreen {
width: 100%;
height: 100%;
font-family: "Josefin Sans", sans-serif;
.upperScreen {
background-image: linear-gradient(
to right,
rgb(124, 50, 131),
rgb(64, 57, 128)
),
url("../../../img/background.jpg");
background-blend-mode: screen;
background-repeat: no-repeat;
background-position: center;
height: 300px;
width: 100%;
display: flex;
justify-content: center;
.middleScreen {
margin-top: 50px;
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Es más recomendable tener una estructura de estilos más plana, sin que estén anidados.

.mainScreen {
  width: 100%;
  height: 100%;
  font-family: "Josefin Sans", sans-serif;
}

.upperScreen {
  background-image: linear-gradient(
      to right,
      rgb(124, 50, 131),
      rgb(64, 57, 128)
    ),
    url("../../../img/background.jpg");
  background-blend-mode: screen;
  background-repeat: no-repeat;
  background-position: center;
  height: 300px;
  width: 100%;
  display: flex;
  justify-content: center;
}

.middleScreen {
  margin-top: 50px;
}


const activeList = todoList.filter((todo) => todo.completed === false);

if (location.pathname === "/active" && activeList.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sería más recomendable pasar la lista de todos ya filtrados a cada página sin tener que comprobar el location.pathname dentro de este componente.

type="button"
className={theme === "dark" ? clickedClass : ""}
id="darkMode"
onClick={(e) => switchTheme(e)}

Choose a reason for hiding this comment

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

No need to use the arrow function. Simply onClick={switchTheme} will work

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.

4 participants