-
Notifications
You must be signed in to change notification settings - Fork 226
Template element #384
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
Template element #384
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muy, bueno, encontré solo un acento y un signo ! de apertura
Me permití arreglar los números de linea. Necesitamos que coincidan para facilitar la review y epecialmente los sync con el repo inglés.
¡AY! casi se me escapan: Acá no afectan porque en el ejemplo no lo usan, pero:
no se traducen variables, ni classes y id del html (se llama refactoring ja!)
podrian ntroducir fallos, especialmente si se usaran en css y js externos
Please make the requested changes. After it, add a comment "/done". |
Co-authored-by: joaquinelio <joaquinelio@gmail.com>
Co-authored-by: joaquinelio <joaquinelio@gmail.com>
Co-authored-by: joaquinelio <joaquinelio@gmail.com>
Co-authored-by: joaquinelio <joaquinelio@gmail.com>
Co-authored-by: joaquinelio <joaquinelio@gmail.com>
Co-authored-by: joaquinelio <joaquinelio@gmail.com>
/done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Co-authored-by: joaquinelio <joaquinelio@gmail.com>
👍 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Muy bueno, corregí algunas estructuras de frases y eliminé comillas que no eran necesarias. Por lo demás, correcto. Esperaré los cambios para aprobarlo
Please make the requested changes. After it, add a comment "/done". |
Hola @RocioC1207 ! Te he dejado unas pequeñas correcciones para que las revises y podamos aprobar tu PR, en cuanto te sea posible, sólo queremos saber si aún sigues activa en la repo. Gracias! |
@joaquinelio Revisando este PR me di cuenta que viene de la rama "master"... ¿Afectará en algo eso? Creo que @RocioC1207 deberá rehacer el PR desde una nueva rama como hemos sugerido en el README |
Que sea SU master No nos afecta. |
FIRME recomendación es mantener el master propio limpio, pero solo es comodidad para uno. Entonces: Tenes TU fork Al Modificar el master Pero al PR no le importa de donde vienen los commits. Edito: Pero un nuevo PR sería sobre una base algo distinta, el cambio no se envía de nuevo Como sea, modificar master local es solo un incordio individual. |
Y si no aparece votaria por merge igual, |
No afectará en este PR. Para el próximo PR que cree sí afectará, porque creará nueva rama desde master y esta contendrá los commits que master tenga. Por ello la recomendación en README. Lo mejor es que una vez aprobado este PR, @RocioC1207 elimine su fork y su repo local, y vuelva a crearlo desde 0. Así su master estará limpio y comenzará siguiendo nuestra recomendación: Nueva rama para cada nuevo PR |
Ah, perdón @vplentinax, contestaba a "si deberá rehacer el PR" y no, no haria falta. Tambien puse "se arregla fácil" no puse cómo git reset si trabaja local, si edita online ni idea, pero en ese caso borrar y rehacer el fork despues de este PR sería muy simple. |
Y parece que @RocioC1207 se cansó de nosotros... =) |
Co-authored-by: Valentina VP <34555644+vplentinax@users.noreply.github.com>
Co-authored-by: Valentina VP <34555644+vplentinax@users.noreply.github.com>
Co-authored-by: Valentina VP <34555644+vplentinax@users.noreply.github.com>
Co-authored-by: Valentina VP <34555644+vplentinax@users.noreply.github.com>
Hola! |
/done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
@danilobrinu gracias por tomarte el tiempo de leerlo En este caso habia los dos reviews, una completa y la otra en proceso, por eso no di merge a pesar de tener tu aprobacion Tambien notaras que el bot no te registra. Los maintainers sí, a veces los pr quedan demorados por falta de review asi que siempre es bueno tener uno más que revise. |
Thank you 💖 I updated the Progress Issue #17 🎉 🎉 🎉 |
@joaquinelio sii recien me di cuenta, por cierto ya estoy libre para apoyar en los reviews :D |
@danilobrinu ok! |
Hola! Espero sus correcciones.
Saludos!