Skip to content
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

Boton para compartir tonto del dia #76

Merged
merged 10 commits into from
Apr 26, 2024
Merged

Conversation

JLeonN
Copy link
Contributor

@JLeonN JLeonN commented Apr 22, 2024

No description provided.

@JLeonN JLeonN requested a review from francosang April 22, 2024 23:08
@JLeonN JLeonN requested a review from francosang April 23, 2024 10:32
const TarjetaTonta = ({ nombre, total, titulos }) => {
import { Link } from "react-router-dom";

const TarjetaTonta = ({ nombre, total, titulos, mensaje, todaLaInfo }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Creo que hay dos variables para resolver un mismo problema.
Esto se podria resolver con una sola variable.

Te explico mas adelante.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hay dos opciones para mejorar esto: borrar la variable todaLaInfo o borrar la variable mensaje.
Pero una de las variables se tiene que ir.
No se pueden quedar las dos porque es confuso y no queda claro.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Opcion 1: Borrar todaLaInfo

Si se borra la variable todaLaInfo, el componente Tarjeta ya no va a ser responsable de decidir si arma el mensaje del dia o el mensaje con toda la info.

Como solo quedaria la variable mensaje, seria la responsabilidad de cada pantalla construir el mensaje que quiere mostrar en la tarjeta.

Opcion 2: Borrar mensaje

Si se borra la variable mensaje, el componente tarjeta va a tener toda la responsabilidad de armar el mensaje.

Dependiendo de todaLaInfo, el componente Tarjeta va a armar el mensaje del dia o el mensaje de toda la info.

Como la variable mensaje ya no existiria, las pantallas que usen la Tarjeta no van a poder mandar un valor de mensaje, solo van a poder decidir si mostrar toda la info o no.

Comment on lines 32 to 33
mensaje="Cowboy del día de hoy es "
todaLaInfo=""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Estas pasando dos variables:

  • mensaje: con el texto que va a estar en el mensaje
  • todaLaInfo: que la usas solo para controlar que tipo de mensaje mandas, si es nulo mandas el mensaje chiquito y si es "" mandas el mensaje grande.

Creo que se podria usar una sola varaiable.
Hay que decidir si usas solo la variable mensaje o solo la variable todaLaInfo.

@@ -19,6 +19,8 @@ const EstructuraDePerfil = () => {
nombre={participante.nombre}
total={participante.total}
titulos={participante.titulos}
mensaje="Tonto seleccionado: "
todaLaInfo={participante.total + participante.titulos}
Copy link
Collaborator

Choose a reason for hiding this comment

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

No tiene sentido ponerle valores a variable todaLaInfo porque el componente Tarjeta nunca usa el valor de esta variable.

La tarjeta solo compara si todaLaInfo es igual a "".

En todo caso, si se queda la variable todaLaInfo deberia ser booleana.

Borré un <div> de más que había. También le di un poco de margin-top: 20px.
@JLeonN JLeonN requested a review from francosang April 25, 2024 01:12
Comment on lines 5 to 27
if (todaLaInfo == "perfil") {
laInfo =
"Tonto seleccionado: " +
nombre +
"%0A" +
"Veces tonto: " +
total +
"%0A" +
"Titulos: " +
titulos;
}
if (todaLaInfo == "tontoDelDía") {
laInfo = "Tonto del día de hoy es " + nombre;
}
if (todaLaInfo == "CowboyDelDía") {
laInfo = "Cowboy del día de hoy es " + nombre;
}
if (todaLaInfo == "errorTonto") {
laInfo = "El algoritmo está preparando un tontos " + nombre;
}
if (todaLaInfo == "errorCowboy") {
laInfo = "El algoritmo está preparando un cowboy " + nombre;
}
Copy link
Collaborator

@francosang francosang Apr 25, 2024

Choose a reason for hiding this comment

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

Esaaaaa! Me gusta! La variable ahora controla el tipo de mensaje.

Habría que cambiarle el nombre, a algo como:

  • tipoDeMensaje
  • mensajeTipo
  • o tipo
  • o algo asi

Que te parece?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Si es verdad

if (todaLaInfo == "tontoDelDía") {
laInfo = "Tonto del día de hoy es " + nombre;
}
if (todaLaInfo == "CowboyDelDía") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Estaba viendo que los otros valores, como perfil o tontoDelDia o errorTonto, empiezan con minuscula.
Pero CowboyDelDia empieza con mayuscula.

Mejor cambiarlo a cowboyDelDia.
Te parece bien?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

jaja
si claro
no me di cuenta

Estoy cambiando los nombres de algunas variables para que el código sea más legible y fácil de entender.  💻
@JLeonN JLeonN requested a review from francosang April 25, 2024 10:05
francosang
francosang previously approved these changes Apr 25, 2024
@francosang
Copy link
Collaborator

Vamo @JLeonN ! Gracias por el laburo booo!

@JLeonN JLeonN merged commit 062cbfc into main Apr 26, 2024
1 check passed
@JLeonN JLeonN deleted the Boton-para-compartir-tonto-del-dia branch April 26, 2024 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants