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

feat(DSFR): migration des pages fiches service public #6277

Merged
merged 37 commits into from
Dec 17, 2024

Conversation

carolineBda
Copy link
Contributor

@carolineBda carolineBda commented Nov 12, 2024

Fix #6232

FicheServicePublicDoc,
} from "@socialgouv/cdtn-types";
import { FicheSPData } from "../../fiche-service-public/type";

Copy link
Contributor Author

@carolineBda carolineBda Nov 19, 2024

Choose a reason for hiding this comment

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

type à exporter dans cdtn-admin

Copy link
Contributor Author

Choose a reason for hiding this comment

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

type à exporter dans l'cdtn-admin

Copy link
Member

@maxgfr maxgfr left a comment

Choose a reason for hiding this comment

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

GG car le code est vraiment pas simple.... Je pense on peut opti, après avec la nouvelle version de React, ce n'est pas forcement nécessaire

Par ailleurs, truc auquel j'y pense ça serait pas mal de voir si avec les nouvelles versions des données des fiches sp, on récupérerait pas l'html comme avec ministère du travail où on scrap. ca nous permettrait d'éviter d'utiliser ce code complexe.

A-t-on fait une étude pour creuser le point d'entrée des données ?

Comment on lines 28 to 43
const accordionItems = data.children
.filter(isItemOfAccordion)
.map((accordionItem: FicheSPDataChapitre | FicheSPDataCas) => {
const title = getTitleInChildren(accordionItem);
const content = (
<ElementBuilder
data={filterOutTitle(accordionItem)}
headingLevel={headingLevel + 1}
/>
);
return {
content,
title,
};
});

Copy link
Member

Choose a reason for hiding this comment

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

je pense qu'on peut optimiser la génération via un useMemo, car là à chaque changement de composant, ça se recalcule

Copy link
Contributor

Choose a reason for hiding this comment

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

Il n'y a pas de changement à ce niveau là. On va faire un premier render et après il n'y a pas de nouveau render car pas de modification du dom par la suite (on ne fait que générer du HTML sans interaction utilisateur). Ce sont des pages candidates pour l'utilisation d'un cache ou une génération static du HTML.

Comment on lines 17 to 23
const formatedDate =
(data.attributes?.date &&
format(parseISO(data.attributes.date), "dd MMMM yyyy", {
locale: frLocale,
})) ||
undefined;
const title = getInChildrenByName(data, "Titre");
Copy link
Member

Choose a reason for hiding this comment

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

là aussi, je pense on peut optimiser ces données d'entrées. après avec la nouvelle version de react : https://www.developerway.com/posts/react-compiler-soon et le nouveau compilateur on en aura pas besoin. mais dans les faits, c'est vrai que ça serait bien d'avoir de l'opti sur ce genrte de composant.

en sachant que la nouvelle version gère ça, on peut laisser comme ça

Copy link
Member

Choose a reason for hiding this comment

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

est-ce que ça vaut le coup d'avoir tout ces composants snapshoté ? serait-il pas mieux de faire la génération sur un seul composant pour éviter la redondance

Copy link
Member

Choose a reason for hiding this comment

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

Je te cache pas que la gestion des types est compliqué et je comprends pq tu n'es pas sûr. On a difficlement de la visibilité dessus, donc je pense on peut laisser comme ça

Copy link
Contributor Author

Choose a reason for hiding this comment

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

merci pour ton retour !

@carolineBda carolineBda marked this pull request as ready for review December 12, 2024 14:46
const url = data.attributes.URL;
const title = getText(data.children[0]);
return (
<Link href={url} target="_blank" rel="noopener noreferrer">
Copy link
Contributor

Choose a reason for hiding this comment

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

juste pour ne pas oublier. il faudra utiliser notre composant Link pour cette partie

const { container } = render(
<ReferenceList references={references as ServicePublicReference[]} />
);
expect(container).toMatchSnapshot();
Copy link
Contributor

Choose a reason for hiding this comment

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

:'(

Copy link
Contributor

Choose a reason for hiding this comment

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

J'aurais testé les fonctionnalités et les labels soient correctes selon le type de la référence.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pas bloquant du tout :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t'as raison 👍 j'ai mis à jour

@carolineBda carolineBda merged commit 85918d7 into dev Dec 17, 2024
22 of 23 checks passed
@carolineBda carolineBda deleted the carolineBda/dsfr-fiche-SP branch December 17, 2024 11:26
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.

Fiche service-public
5 participants