feat(bootstrap, react/homepage): #COCO-5441, school widget#468
feat(bootstrap, react/homepage): #COCO-5441, school widget#468jcbe-ode wants to merge 13 commits intoepic-homepagefrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new “SchoolWidget” for the React homepage module (with styling, Storybook stories, and a helper hook) and updates the client School session type to reflect nullable exports.
Changes:
- Introduce
SchoolWidgetcomponent + Storybook stories for single/multiple/empty states. - Add
useUserSchoolshook to derive schools/selected school fromEdificeClientProvider. - Update
School.exportstyping in the client session interfaces (string[] | null) and add widget CSS/SVG background.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/modules/homepage/components/SchoolWidget/useUserSchools.ts | New hook to read user schools and manage selected school state. |
| packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidget.tsx | New UI widget rendering selected school + dropdown when multiple schools. |
| packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidget.stories.tsx | Storybook coverage for multiple/single/empty scenarios. |
| packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidget.css | Widget styling + CSS variables + background image binding. |
| packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidgetBackground.svg | Background asset for the widget. |
| packages/client/src/ts/session/interfaces.ts | Update School.exports to allow null from the API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/react/src/modules/homepage/components/SchoolSpace/SchoolSpace.tsx
Show resolved
Hide resolved
packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidget.css
Outdated
Show resolved
Hide resolved
packages/react/src/modules/homepage/components/SchoolSpace/useUserSchools.ts
Show resolved
Hide resolved
packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidget.tsx
Outdated
Show resolved
Hide resolved
packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidget.tsx
Outdated
Show resolved
Hide resolved
damienromito
left a comment
There was a problem hiding this comment.
Une décision à prendre sur l'endroit où mettre le style. Discussion à avoir avec @pascalsaussier-edifice
En terme de design j'attend confirmation de Simon https://www.figma.com/design/UbAKNC5vk5XOj62z6CI4Qm?node-id=2146-36161#1690237762
Mais ça sera probablement :
- si le texte est plus grand que le container, ajouter un elipsis au lieu de passer sur deux le lignes
- le background doit être sticky en haut du block et pas en bas
|
|
||
| const hasManySchools = schools && schools.length > 1; | ||
|
|
||
| const widgetStyle = { padding: '1.4rem 0.4rem' }; |
There was a problem hiding this comment.
mettre le css en dehors du composant et au même endroit, dans le fichier .scss (cf commentaire precedent). C'est un point important à acter pour le rester du projet
There was a problem hiding this comment.
Tout ce style doit être dans un fichier CSS ou SCSS.
Les seuls styles inlines qu'on va s'autoriser sont ceux qui sont calculés en fonction d'un state ou d'une variable du composant.
| const widgetStyle = { padding: '1.4rem 0.4rem' }; | ||
| const containerStyle = { padding: '0.8rem' }; | ||
| const selectedSchoolStyle = { | ||
| padding: '.4rem 2.9rem', |
There was a problem hiding this comment.
pour les espacement, dans les application on utiliser les classes bootstrap (p-8, p12,...), dans le FF il faut utiliser les spacer (qui ne peut que être utilisé dans les fichiers sass dans bootstrap justement, exemple)
Ici il s'agit d'un spacer 1.2 (12pixel sur figma)
| color="tertiary" | ||
| variant="ghost" | ||
| icon={ | ||
| <IconRafterDown |
There was a problem hiding this comment.
wahou chiant d'avoir dû reimplementer ça. J'ai fait un commentaire à simon https://www.figma.com/design/UbAKNC5vk5XOj62z6CI4Qm?node-id=2146-36161#1690319223
| export const Empty: Story = { | ||
| render: renderWithProps({ selectedSchool: undefined }), | ||
| parameters: { | ||
| docs: { | ||
| description: { | ||
| story: `État vide (aucune école)`, | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
J'aime bien avoir la possibilité de voir les Edge Case dans storybook, ça retire pas mal de questionnements quand on regarde le composant
There was a problem hiding this comment.
est ce que le cas avec aucune ecole est possible ?
There was a problem hiding this comment.
Non, mais en cas d'erreur de paramétrage (provenant de la BDD) autant que le widget ne crashe pas la page !
| @@ -0,0 +1,122 @@ | |||
| // packages/react/src/modules/homepage/components/SchoolWidget/useUserSchools.test.tsx | |||
There was a problem hiding this comment.
Comme pour le style, je pense qu'a la compilation , l'image ne sera pas ajoutée aux assets (tester en utilisant le composant dans Actualites par exemple).
Je vois qu'il y a un dossier react/modules/icons , sinon ajouter dans le dossier /bootstrap.
Quelle est la bonne pratique @pascalsaussier-edifice ?
| setSelectedSchool( | ||
| index < 0 || index >= schools.length ? schools[0] : schools[index], | ||
| ); | ||
| } else { |
There was a problem hiding this comment.
est ce vraiment utile de gerer ce cas ? c'est possible fonctionnellement qu'il passe à undefined ?
There was a problem hiding this comment.
- L'utilisateur choisit une structure => le choix est enregistré en BDD
- Une année passe, ou bien il changé d'établissement en cours d'année
=> à la reconnexion, le choix précédemment enregistré n'est plus disponible dans la liste
==> on affiche le 1er étab de la liste à la place
| border-radius: 2.4rem; | ||
|
|
||
| background-color: var(--school-widget-bg-color); | ||
| background-image: url('./SchoolWidgetBackground.svg'); |
There was a problem hiding this comment.
C'est une image qui va être lié aux préférénces de couleur qui seront accessibles dans le panneau de configuration. Pas en v1 mais faudra y penser.
There was a problem hiding this comment.
Ah. bien vu, mais c'est un comportement qu'on va devoir mettre en place à plusieurs endroit. Tu as une idée de comment faire ? on peut passer des proriétés à un svg ?
| export const Empty: Story = { | ||
| render: renderWithProps({ selectedSchool: undefined }), | ||
| parameters: { | ||
| docs: { | ||
| description: { | ||
| story: `État vide (aucune école)`, | ||
| }, | ||
| }, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
J'aime bien avoir la possibilité de voir les Edge Case dans storybook, ça retire pas mal de questionnements quand on regarde le composant
|
|
||
| const hasManySchools = schools && schools.length > 1; | ||
|
|
||
| const widgetStyle = { padding: '1.4rem 0.4rem' }; |
There was a problem hiding this comment.
Tout ce style doit être dans un fichier CSS ou SCSS.
Les seuls styles inlines qu'on va s'autoriser sont ceux qui sont calculés en fonction d'un state ou d'une variable du composant.
packages/react/src/modules/homepage/components/SchoolWidget/SchoolWidget.tsx
Outdated
Show resolved
Hide resolved
| cdnDomain?: string | null; | ||
| version?: string | null; | ||
| } | ||
|
|
There was a problem hiding this comment.
Merci pour le nettoyage !
| globals: true, | ||
| environment: 'jsdom', | ||
| include: ['src/**/*.spec.tsx'], | ||
| include: ['src/**/*.spec.tsx', 'src/**/*.spec.ts'], |
There was a problem hiding this comment.
Question pour plus tard, sur les sites on utilise plutôt .test.ts[x] le .spec.ts[x] c'est surtout un héritage d'Angular, est-ce que ça vaudrait pas le coup de normaliser avec le .test ?
@damienromito
There was a problem hiding this comment.
pourquoi pas, test est plus explicite en effet, mais il faudra le changer partout ^^
07e420b to
62004ee
Compare
62004ee to
5874ab8
Compare
9248f7f to
dcae84e
Compare
| @use '../../vendors/bootstrap'; | ||
|
|
||
| .school-widget { | ||
| --border-color: #ffe5a3; |
There was a problem hiding this comment.
Comme les variables sont scopées, on peut raccourcir leur nom sans risquer la collision avec les variables des autres composants.
There was a problem hiding this comment.
oh pas bête. Comme pour des variables en JS je ne trouve pas forcement utile quand on a une seule instance d'une variable, mais ça n'affecte pas la lisibilité donc ça me va
|
Tous les retours sont traités autant que possible. |
damienromito
left a comment
There was a problem hiding this comment.
rien de bloquant ! top, un bon exemple de composant
| @use '../../vendors/bootstrap'; | ||
|
|
||
| .school-widget { | ||
| --border-color: #ffe5a3; |
There was a problem hiding this comment.
oh pas bête. Comme pour des variables en JS je ne trouve pas forcement utile quand on a une seule instance d'une variable, mais ça n'affecte pas la lisibilité donc ça me va
| @use '../../abstracts/' as *; | ||
| @use '../../vendors/bootstrap'; | ||
|
|
||
| .school-space { |
There was a problem hiding this comment.
reflexion : Est ce qu'on ajouterait pas "homepage" pour tous les composant de la home => "homepage-school-space" ?
| export * from './SchoolSpace'; | ||
| export { default as SchoolSpace } from './SchoolSpace'; | ||
| export * from './SchoolSpaceContainer'; | ||
| export * from './useUserSchools'; |
Description
Adds a new
SchoolSpacecomponent for the React homepage module (with styling, Storybook stories, and tests).Adds a hook to help populating the widget properties.
Adds a container encapsulating both the hook and component.
Adds a
useWidgetPreferencesfor the homepage to load users preferences about widgets.Updates the client School session type to reflect nullable exports (as seen during local tests).
Ticket https://edifice-community.atlassian.net/browse/COCO-5441
Which Package changed?
Please check the name of the package you changed
Has the documentation changed?
Type of change
Please check options that are relevant.
Checklist: