-
Notifications
You must be signed in to change notification settings - Fork 22
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
feat(agreement): add new agreement page #1840
Conversation
f159a96
to
981928f
Compare
const theme = themes.find(theme => theme.refs.some(themeFinder)); | ||
return { | ||
question: value, | ||
answer: compiler.processSync(answer.markdown).contents, |
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.
warn: in the markdown we can find some MDX content (Content, Section...)
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.
so it may be processed at runtime using @nkrmr work
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.
je pense que l'idée de départ est plutôt d'avoir un aperçu de la réponse et de renvoyer vers la réponse complete
{nbArticles.vigueurEtendu + nbArticles.vigueurNonEtendu > 0 | ||
? "(" | ||
: ""} | ||
{nbArticles.vigueurEtendu > 1 |
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.
>=
?
{nbArticles.vigueurEtendu > 0 && nbArticles.vigueurNonEtendu > 0 | ||
? ", " | ||
: null} | ||
{nbArticles.vigueurNonEtendu > 1 |
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.
>=
?
Hello @lionelB merci pour la PR ! Quelques remarques en vrac : intro
articles par thème
recherche
contributions
général
|
<tr> | ||
<th>Nombre d’articles</th> | ||
<td> | ||
{nbArticles.vigueurEtendu + nbArticles.vigueurNonEtendu} |
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.
A mon avis la pluralize
fonction est un peu "too smart for her own good" et trop spécifique pour être vraiment générisée dans lib
pour le moment (le cas ou on a besoin du chiffre + des pluriels sont inexistants dans le proj à part ici). Mais je suis d'accord qu'une fonction non générique (qui resterait dans ce fichier) qui permette d'avoir des noms de variable plus courts (dans l'idée de pluralize avec nb
) pour faciliter la lecture et la compréhension de la logique pour générer le wording ici ferait du bien ! Il y a pas mal d'opérateurs ternaires et de check qui se suivent, c'est pas évident de comprendre sans un code plus court / un exemple.
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.
pour l'utilité de pluralize , je pense qu'en cherchant un petit peu il y a plusieurs endroit qui pourrait en bénéficier comme par exemple marque Source / Sources en fonction du nombre d'article cité dans les pages Résultats des simulateurs.
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.
Une fois que les retours de @ClementChapalain et @revolunet sont traités
@lionelB commentaire updated |
981928f
to
3f69bb9
Compare
Retours V2 :) :
|
5ad6a64
to
3f87ff9
Compare
3f87ff9
to
0789a84
Compare
Add new agreement page
UI is still a bit wip
fix #1398