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

Template element #384

Merged
merged 13 commits into from
Oct 20, 2020
Merged

Template element #384

merged 13 commits into from
Oct 20, 2020

Conversation

RocioC1207
Copy link
Contributor

Hola! Espero sus correcciones.
Saludos!

@CLAassistant
Copy link

CLAassistant commented Sep 21, 2020

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@joaquinelio joaquinelio left a 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

8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
@javascript-translate-bot

Please make the requested changes. After it, add a comment "/done".
Then I'll ask for a new review 👻

RocioC1207 and others added 6 commits September 21, 2020 20:51
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>
@RocioC1207
Copy link
Contributor Author

/done

Copy link
Member

@joaquinelio joaquinelio left a 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>
@joaquinelio
Copy link
Member

👍 👍
segunda review?
no estaría mal una de una frontend... @vplentinax

Copy link
Contributor

@vplentinax vplentinax left a 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

8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
8-web-components/4-template-element/article.md Outdated Show resolved Hide resolved
@javascript-translate-bot

Please make the requested changes. After it, add a comment "/done".
Then I'll ask for a new review 👻

@vplentinax
Copy link
Contributor

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!

@vplentinax
Copy link
Contributor

@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

@joaquinelio
Copy link
Member

Que sea SU master No nos afecta.

@joaquinelio
Copy link
Member

joaquinelio commented Oct 7, 2020

FIRME recomendación es mantener el master propio limpio, pero solo es comodidad para uno.

Entonces: Tenes TU fork
Mantenes tu master limpio y actualizado,
haces branch, alteras branch (y te lo alteran si los dejas) y si se acepta y se incorpora al repo, ya podes borrar el branch con confianza.
Luego en TU MASTER solamente haces un pull y todo queda atualizado y prolijo,

Al Modificar el master
No podes hacer simultáneamente una segunda modificacion sin tener la basura de la primera
Si algo se modifica, un pull puede no dejarlos iguales (ejempl si se rechaza una de las modificaciones),
Se arregla facil (ja, me enseñaste cómo) pero MUCHO mas facil es trabajar con diferentess branchs

Pero al PR no le importa de donde vienen los commits.

Edito:
Me imagino que ssi modificaste master, y enviaste PR, se alteran y hace merge, haces pull, podria quedar sin deshacer el cambio que UPSTREAM rechazó.

Pero un nuevo PR sería sobre una base algo distinta, el cambio no se envía de nuevo
pero si lo hiciera sería visible para el reviewer.

Como sea, modificar master local es solo un incordio individual.

@joaquinelio
Copy link
Member

Y si no aparece votaria por merge igual,
De acuerdo con quitar comillas a "hijos" que distraen y remarcan algo que deberia leerse naturalmente,
las otras suggest no las entendi del todo, apruebo con o sin

@vplentinax
Copy link
Contributor

Que sea SU master No nos afecta.

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

@joaquinelio
Copy link
Member

joaquinelio commented Oct 9, 2020

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 --hard upstream/master
aunque NO es buena práctica acotumbrarse a los reset, deberia ser algo excepcional.
Lo mejor es siempre trabajar branchs, todos los que quieras, y no borrarlos hasta que sabes que ya no los necesitas.

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.
jajaja "seria muy simple" y tampoco puse cómo... es que no me fijé. Pero debe serlo, ¿no? =)

@joaquinelio
Copy link
Member

Y parece que @RocioC1207 se cansó de nosotros...

=)

RocioC1207 and others added 4 commits October 15, 2020 18:58
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>
@RocioC1207
Copy link
Contributor Author

Hola!
Quiero disculparme por mi ausencia, tuve un mes complicado.
Gracias por las correcciones!

@RocioC1207
Copy link
Contributor Author

/done

Copy link
Contributor

@vplentinax vplentinax left a comment

Choose a reason for hiding this comment

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

Ok

@joaquinelio
Copy link
Member

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

@joaquinelio joaquinelio merged commit 3ba1b6f into javascript-tutorial:master Oct 20, 2020
@javascript-translate-bot

Thank you 💖 I updated the Progress Issue #17 🎉 🎉 🎉

@dbritto-dev
Copy link
Contributor

@joaquinelio sii recien me di cuenta, por cierto ya estoy libre para apoyar en los reviews :D

@joaquinelio
Copy link
Member

@danilobrinu ok!
Fui flexibilizando con el tiempo, algunas cosas son regionales y se pueden mejorar, otras pueden quedar, unos dicen accesar y otros acceder, unos traducen array y otros no, algunas cosas hacen a la fluidez, salvo que sea ortografia evidente siempre pongo el porqué
No se tu expertise, el problema ahora lo tengo con cosas conceptuales, huelo los problemas de código pero no trabajé en produccion con html, async, net request.
Pero hassta la correccion de typos es útil.

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

Successfully merging this pull request may close these issues.

None yet

6 participants