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

No más warnings #968

Merged
merged 9 commits into from
Apr 19, 2022
Merged

No más warnings #968

merged 9 commits into from
Apr 19, 2022

Conversation

dlopezalvas
Copy link
Contributor

@dlopezalvas dlopezalvas commented Apr 18, 2022

Fixes #922

Resulta que existe una cosa mágica llamada Sass migrator que se encarga de cambiar esos / por math.div, que era lo que estaba haciendo a manopla en el proyecto pilas-bloques-angular-material-styles.

Habían tres casos en los que teníamos esta clase de warnings:

  • En angular-material-styles que usa Ember Paper
  • En font-awesome que usa ember-font-awesome (que no encontré el repo).
  • En una línea en Pilas Bloques

En font-awesome hay un fix en una versión más alta aparentemente: FortAwesome/Font-Awesome#17946 peeero pasa algo similar con angular-material-styles (no encuentro el repo de ember en donde usan a font-awesome D: )

Me pareció que podemos aprovechar sass-migrator en lugar de mantener los proyectos pilas-bloques-ember-paper y pilas-bloques-angular-material-styles, y buscar una manera de solucionar lo mismo con font-awesome. Arme un script que se corre después del install que hace el cambiazo de tanto angular-material-styles como font-awesome (en los node_modules) en cada lugar que se use /.

¿Les parece?

Aprovecho que es un PR de warnings de paso para sacar un warning sobre las traducciones donde se nos paso (probablemente a mí) cambiar el false por other.

@dlopezalvas dlopezalvas marked this pull request as ready for review April 18, 2022 17:14
@dlopezalvas dlopezalvas requested a review from a team as a code owner April 18, 2022 17:14
Copy link
Contributor

@tfloxolodeiro tfloxolodeiro left a comment

Choose a reason for hiding this comment

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

😱😱😱😱

Muy bien yendo con el approach ingenieril de buscar si otra persona ya resolvio esto antes genial excelente increible perfecto sobresaliente extraordinario barbaro.

Como decis, esto tambien nos resuelve lo de ember-font-awesome que descubriste. Por lo que busque, el repo de ember-font-awesome FUE BORRADO, asi que lo que hiciste es lo unico que podemos hacer para arreglar el warning de ember-font-awesome, porque nisiquiera podriamos hacer un pilas-bloques-ember-font-awesome-aaaa.

Considerando que ember-font-awesome ya NO EXISTE, podriamos ver que tan doloroso puede ser cambiar a esta otra dependencia que esta maso activa, pero eso en otro momento.

Copy link
Contributor

@asanzo asanzo left a comment

Choose a reason for hiding this comment

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

ah-screaming

Pucha que leveleaste, Diana.

  • Entendiste el problema de dependencias y de mantenimiento del proyecto, intentaste y te chocaste con varias posibles soluciones.
  • Entendiste los momentos de compilación/construcción de npm, y propusiste una solución en el postinstall (hackerosa, sí, pero bastante mágica también).
  • Esa solución podría funcionar para posibles updates (o podría romperse) pero aún así es la mejor de todas las que pensamos

👏 👏 👏

Salvo un comment que dejé por ahí sobre un decimal, para mí está para mergearse (si querés dejé otras propuestas tmb)

app/styles/breadcrumb.scss Outdated Show resolved Hide resolved
package.json Outdated
"pilasweb": "^0.5.0",
"unzipit": "^1.3.5",
"proceds-blockly": "^1.1.0",
"sass-migrator": "^1.5.4",
Copy link
Contributor

Choose a reason for hiding this comment

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

¿No debería ser dev-dependencies?

scripts/change-sass-division.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@ezequielPereyra ezequielPereyra left a comment

Choose a reason for hiding this comment

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

Ember:

Todo lo que toco perece!
image

@@ -20,7 +20,7 @@ $breadcrumb-size: 30px;
margin-left: -$breadcrumb-size;
padding: 5px;
padding-left: $breadcrumb-size + 2;
padding-right: $breadcrumb-size / 2;
padding-right: $breadcrumb-size * 0,5;
Copy link
Contributor

Choose a reason for hiding this comment

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

image

@asanzo
Copy link
Contributor

asanzo commented Apr 18, 2022

@tfloxolodeiro te cargás el issue con lo que mencionás, porfa? que incluya la descripción sobre modificar el sass migrator script tmb

@asanzo asanzo merged commit feab134 into develop Apr 19, 2022
@asanzo asanzo deleted the ember-warning branch April 19, 2022 15:08
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.

Warnings de ember-paper
4 participants