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

Format les vues .html.erb avec erb_format #404

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jbfeldis
Copy link
Contributor

@jbfeldis jbfeldis commented Sep 3, 2024

J'ouvre en draft pour discuter l'utilisation d'erb_format pour formatter nos vues eruby.

En lançant l'utilitaire sur l'ensemble de nos vues pour cette PR il a d'ailleurs planté 3 fois sur des fichiers malformés (des soucis de clôture de div et de tr je crois) 😅

@skelz0r
Copy link
Member

skelz0r commented Sep 3, 2024

En lançant l'utilitaire sur l'ensemble de nos vues pour cette PR il a d'ailleurs planté 3 fois sur des fichiers malformés (des soucis de clôture de div et de tr je crois) 😅

which is nice !

Comment on lines +22 to +27
<%= link_to t(".cancel"),
"#",
class: %w[fr-btn fr-btn--secondary],
aria: {
controls: "main-modal",
} %>
Copy link
Member

Choose a reason for hiding this comment

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

ça c'est moche.

Possible de linter comme ça:

Suggested change
<%= link_to t(".cancel"),
"#",
class: %w[fr-btn fr-btn--secondary],
aria: {
controls: "main-modal",
} %>
<%=
link_to(
t(".cancel"),
"#",
class: %w[fr-btn fr-btn--secondary],
aria: {
controls: "main-modal",
}
)
%>

Si non, est-ce que ce que je propose passe le linter ?

Copy link
Member

Choose a reason for hiding this comment

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

Réponse: non.

Il me sort ça à la place:

            <%= link_to(
              t(".cancel"),
              "#",
              class: %w[fr-btn fr-btn--secondary],
              aria: {
                controls: "main-modal",
              },
            ) %>

Comment on lines +22 to +24
<%= link_to t(".cta"),
new_authorization_request_path(id: authorization_definition.id.dasherize),
class: %w[fr-btn fr-btn--sm] %>
Copy link
Member

Choose a reason for hiding this comment

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

olalala mais c'est illisible ce truc, comment un linter peut faire ça sérieusement ?

Comment on lines +22 to +24
<%= link_to t(".cta"),
new_authorization_request_path(id: authorization_definition.id.dasherize),
class: %w[fr-btn fr-btn--sm] %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<%= link_to t(".cta"),
new_authorization_request_path(id: authorization_definition.id.dasherize),
class: %w[fr-btn fr-btn--sm] %>
<%=
link_to(
t(".cta"),
new_authorization_request_path(id: authorization_definition.id.dasherize),
class: %w[fr-btn fr-btn--sm],
)
%>

ça c'est clairement mieux --^

Comment on lines +30 to +32
<%= dsfr_custom_pictogram(
"pictograms/authorization_requests/#{authorization_request_form.use_case}.svg",
) %>
Copy link
Member

Choose a reason for hiding this comment

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

Je vois qu'avec des parenthèses il fait un peu plus d'efforts, c'est mieux mais pas encore OK

Comment on lines +10 to +21
".#{callout_key}.content",
organization_raison_sociale: current_organization.raison_sociale,
),
{ class: "fr-callout__text fr-my-3w" },
wrapper_tag: "p" %>
<%= link_to t(
".#{callout_key}.cta",
organization_raison_sociale: current_organization.raison_sociale,
),
authorization_request_path(authorization_request),
class: "fr-link fr-btn--icon-right fr-icon-arrow-right-line" %>
Copy link
Member

Choose a reason for hiding this comment

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

je saigne des yeux

Copy link
Member

@skelz0r skelz0r left a comment

Choose a reason for hiding this comment

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

Peux-tu ajouter le fichier de config de l'utilitaire ainsi que la commande docker et/ou la bin/ pour qu'on puisse itérer sur la config. Là c'est un gros non comment il me formate ça, c'est illisible.

@Isalafont
Copy link
Contributor

Peux-tu ajouter le fichier de config de l'utilitaire ainsi que la commande docker et/ou la bin/ pour qu'on puisse itérer sur la config. Là c'est un gros non comment il me formate ça, c'est illisible.

La commande docker serait mieux. les commandes bin de mon côté plante alors que les commandes dockers fonctionnent très bien.

(voir même on peut rajouter la commande directement dans le fichier docker-compose.yml)

@skelz0r
Copy link
Member

skelz0r commented Sep 3, 2024

Pour la veille: https://github.com/threedaymonk/htmlbeautifier

Comment on lines 1 to 10
<div class="reopening__footer fr-p-3w fr-background-alt--grey">
<div class="reopening__content flex-space-between__content fr-mb-3v">
<div>
<p class="reopening__title fr-text-title--blue-france fr-text--md font-weight-700 fr-m-0">
<%= t('.reopen_title') %>
<p
class="
reopening__title fr-text-title--blue-france fr-text--md font-weight-700 fr-m-0
"
>
<%= t(".reopen_title") %>
</p>
Copy link
Contributor

Choose a reason for hiding this comment

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

Il y a certaines choses que je ne comprends pas trop avec ce linter.
Les classes de la div sont sur une seule ligne mais pour le p, c'est en plusieurs lignes...

Comment on lines +23 to 29
<%= link_to t(".show_cta"),
authorization_request_form_path(
form_uid: authorization_request.form.uid,
id: authorization_request.id,
),
class: %w[fr-text--sm fr-icon-arrow-right-line fr-link--icon-right] %>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Par contre, ça j'aime bien !

@skelz0r
Copy link
Member

skelz0r commented Sep 3, 2024

Ceci #404 (comment) peut me convenir (même si c'est pas ultra lisible je trouve). La version d'avant sans les parenthèses ça va être dur, mais bon je ferai du full scout commit ça me dérange pas plus que ça.

Du coup on ferait quoi exactement ici ?

  1. On force le full formatage ?
  2. On vérifie juste que le HTML est valide ?

Pour la CI:

Pour 1.

erb-format --write app/views/**/*.html.erb &> /dev/null
git diff --quiet

Pour 2:

erb-format --no-write app/views/**/*.html.erb &> /dev/null

Le bin:

bundle exec erb-format --write app/views/**/*.html.erb

Le makefile:

html-lint:
        $(DOCKER-RUN) web $(BUNDLE-EXEC) erb-format --write app/views/**/*.html.erb

@jbfeldis
Copy link
Contributor Author

jbfeldis commented Sep 9, 2024

@skelz0r si tout le monde est OK c'est un bon plan

  1. on reformate tout dans (une|cette) PR
  2. on CI le côté valide comme tu indiques erb-format --no-write app/views/**/*.html.erb &> /dev/null
  3. on ajoute le make html-lint ou fix-html vu qu'il écrit, est-ce qu'on ne ferait d'ailleurs pas un make format qui fasse tous les fix d'un coup (rb, js, yml, html.erb) ?

@Isalafont
Copy link
Contributor

3. on ajoute le make html-lint ou fix-html vu qu'il écrit, est-ce qu'on ne ferait d'ailleurs pas un make format qui fasse tous les fix d'un coup (rb, js, yml, html.erb) ?

J'avoue que ça commence à faire beaucoup commande différente à apprendre pour les lints.
On peut garder les commandes individuelles et en faire une qui les inclus toutes ?

@skelz0r
Copy link
Member

skelz0r commented Sep 9, 2024

  1. on ajoute le make html-lint ou fix-html vu qu'il écrit, est-ce qu'on ne ferait d'ailleurs pas un make format qui fasse tous les fix d'un coup (rb, js, yml, html.erb) ?

J'avoue que ça commence à faire beaucoup commande différente à apprendre pour les lints. On peut garder les commandes individuelles et en faire une qui les inclus toutes ?

Be my guest.

Pour ma part je les lance pas je laisse la CI lamentablement fail :D

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.

3 participants