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

Notice type should be mandatory #25

Open
SamGuay opened this issue Mar 21, 2021 · 5 comments
Open

Notice type should be mandatory #25

SamGuay opened this issue Mar 21, 2021 · 5 comments
Labels
bug Something isn't working

Comments

@SamGuay
Copy link
Member

SamGuay commented Mar 21, 2021

Lorsqu'on génère notre propre titre de notice, il n'y a aucune couleur différencier le titre du body de la notice. S'il pouvait y avoir un petit contraste, ce sera 🔥.

{{% notice "Nice Looking Title" %}}
Plain shortcode
{{% /notice %}}

image

@SamGuay SamGuay added the enhancement New feature or request label Mar 21, 2021
@dannycolin
Copy link
Member

@SamGuay tu pourrais partager un snippet du code qui te donne ce résultat?

@SamGuay
Copy link
Member Author

SamGuay commented Mar 21, 2021

Oups, j'avais oublié de l'intégrer. Voir edit ci-haut.

En regardant le code du shortcode:

<div class="notice-wrapper" {{ if len .Params | eq 2 }} id="{{ .Get 1 }}" {{ end }}>
  <p class="notice-title notice-{{ .Get 0 }}">
    <span class="notice-icon fa fa-exclamation-circle" aria-hidden="true"></span>
    {{ .Get 0 }}
  </p>
  <div class="notice-content">
    {{ .Inner }}
  </div>
</div>

On pourrait ajouter un param au shortcode pour qu'on puisse setter la couleur du notice indépendamment du titre, eg:

<div class="notice-wrapper" {{ if len .Params | eq 3 }} id="{{ .Get 2 }}" {{ end }}>
  <p class="notice-title notice-{{ .Get 0 }}">
    <span class="notice-icon fa fa-exclamation-circle" aria-hidden="true"></span>
    {{ .Get 1 }}
  </p>
  <div class="notice-content">
    {{ .Inner }}
  </div>
</div>

Ce code est à améliorer, car si on met juste warning, if len .Params ne fonctionne plus si l'ordre est changé, etc. Bug prévu si qqu'un utilise seulement 2 params (eg, css class + id). On pourrait soit nommer tous les params (mais chiant quand on écrit du contenu) ou seulement nommer le id= si utilisé..

@dannycolin
Copy link
Member

Oups, j'avais oublié de l'intégrer. Voir edit ci-haut.

Ah! C'est que tu ne lui donne pas un "type" (warning, info). Est-ce que c'est réellement un bug dans ce cas-ci :p ?

On pourrait ajouter un param au shortcode pour qu'on puisse setter la couleur du notice indépendamment du titre, eg:

Je pense qu'on devrait limiter ça à des couleurs prédéterminées, car:

  1. On est sûr quelles ont un contraste suffissant
  2. Ça briserait l'association de la couleur à un type de notice (e.g rouge = attention!, bleu = info supplémentaire)

@SamGuay
Copy link
Member Author

SamGuay commented Mar 21, 2021

Ah! C'est que tu ne lui donne pas un "type" (warning, info). Est-ce que c'est réellement un bug dans ce cas-ci :p ?

Oui, car tel que présenté dans l'exemple, le shortcode n'est pas assez intelligent pour détecter si le param qu'on lui passe est un "type" ou un "titre" random. Par contre, ce bug pourrait devenir une feature et je crois qu'on converge vers la même idée ⬇️

Je pense qu'on devrait limiter ça à des couleurs prédéterminées, car...:

Je suis 100% d'accord avec toi. Quand je me relis j'avoue qu'on dirait que je veux dire "Je veux pouvoir mettre n'importe quelle couleur en passant une couleur en param". Ce que je voulais dire, c'était d'avoir 2 params différents, un pour le "type" (warning, info, tip, success, etc.) et un autre pour pouvoir spécifier le titre.

{{% notice info Title %}}
Saviez-vous que...
{{% /notice %}}

L'idéal serait que le shortcode détecte s'il n'y a pas de titre, de mettre le type en tant que titre. Comme i18n fonctionne aussi dans les shortcodes, la traduction ne serait pas un problème également.

@dannycolin dannycolin added bug Something isn't working and removed enhancement New feature or request labels Mar 21, 2021
@dannycolin dannycolin changed the title Style user-generated notice titles Notice type should be mandatory Mar 21, 2021
@dannycolin
Copy link
Member

L'idéal serait que le shortcode détecte s'il n'y a pas de titre, de mettre le type en tant que titre. Comme i18n fonctionne aussi dans les shortcodes, la traduction ne serait pas un problème également.

C'est que les titres personnalisés ne sont pas supportés. J'ai changé cette issue pour régler le problème avec le type. J'en ouvre une autre pour ajouter l'option d'avoir un titre personnalisé.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants