-
Notifications
You must be signed in to change notification settings - Fork 3
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
Feature/form decomposition #223
Conversation
✅ Deploy Preview for ecospheres ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for ecospheres ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Ca se lit bien c'est cool 👏
Dommage qu'on ne puisse pas prendre en compte #178 dans le temps imparti (je pense), car on introduit pas de mal de composants "Options API". Je pense que la Composition API aurait rendu naturellement certains composants moins verbeux.
class ConfigUtils { | ||
config = config | ||
|
||
public static getThemeOptions() { | ||
const options: SelectOption[] = [] | ||
for (const theme of config.themes) { | ||
options.push({ | ||
value: theme.name, | ||
text: theme.name | ||
}) | ||
} | ||
return options | ||
} | ||
|
||
public static getSubthemeOptions(theme: Theme) { | ||
const options: SelectOption[] = [] | ||
if (theme) { | ||
for (const subtheme of theme.subthemes) { | ||
options.push({ | ||
value: subtheme.name, | ||
text: subtheme.name | ||
}) | ||
} | ||
} | ||
return options | ||
} | ||
|
||
public static getThemeByName(name: string): Theme | null { | ||
for (const theme of config.themes) { | ||
if (theme.name === name) { | ||
return theme | ||
} | ||
} | ||
return null | ||
} | ||
} |
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 mettre dans un fichier src/custom/ecospheres/config.ts
par exemple je pense, les notions de Theme ne sont pas génériques il me semble.
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 qu'il faudrait faire une nouvelle PR pour organiser proprement les custom vs generic car cela sort du scope de celle ci qui visait uniquement la decomposition.
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.
src/custom/ecospheres/components/forms/bouquet/BouquetFormRecap.vue
Outdated
Show resolved
Hide resolved
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.
Pas mal de choses custom ES dans ce fichier qui mériterait d'être bougé dans custom
si on suit la logique des composants, ou au moins dans un fichier moins générique que model
.
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.
Il faudrait prendre le temps de réfléchir à une structure adaptée et idéalement tenter de générifier les composants avant de les déplacer le cas échéant vers custom
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.
} | ||
|
||
interface TopicExtras { | ||
['ecospheres:informations']: { |
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 ne sais pas si c'est possible mais utiliser ${config.universe.name}
comme préfixe ici serait idéal. Sinon cf supra, à bouger dans un endroit plus custom.
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.
on avait commence a en parler et justement c'est une notation qui rend un peu difficile la generification, le plus clair serait que l'information ecosphere soit une valeur et non une propriete. Ex :
extra : {
universe: ecosphere
informations {
....
}
}
mais idem il faudra faire une PR
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.
@@ -103,7 +103,7 @@ export default { | |||
} | |||
}, | |||
methods: { | |||
isRelevant(topic: Topic, property: string, value): Boolean { | |||
isRelevant(topic: Topic, property: string, value: string): Boolean { | |||
const topicInformations: { subtheme: string; theme: string }[] = | |||
topic.extras['ecospheres:informations'] |
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.
En profiter pour utiliser ${config.universe.name}
?
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.
theme: string | ||
subtheme: string | ||
}[] | ||
['ecospheres:datasets_properties']: DatasetProperties[] |
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 ne sais pas si c'est possible mais utiliser ${config.universe.name} comme préfixe ici serait idéal. Sinon cf supra, à bouger dans un endroit plus custom.
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.
@@ -87,6 +88,15 @@ | |||
} | |||
} | |||
|
|||
.fr-fieldset { |
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 scoper dans un composant ? Ca me parait un peu dangereux en style global.
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.
cf #223 (comment) , le comportement decrit (avoir une marge autour des champs) me parait suffisament generique pour etre mis par defaut au moins dans un premier temps et eviter que l'on ajoute la marge dans chaque composant
.dsfr_alert { | ||
margin: 20px 0; | ||
} | ||
|
||
textarea { |
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 scoper dans un composant ? Ca me parait un peu dangereux en style global.
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.
dans le cas du fichier main.css et model.ts, j'ai volontairement fait le choix de centraliser sans structurer dans un premier temps. Le but est de voir d'abord comment grossissent ces fichiers pour creer une structure pertinente et eviter d'optimiser/specialiser trop tot.
Le comportement decrit ici (avoir une marge autour du champs pour la lisibilite et le definir par defaut plus grand pour inciter les utilisateurs a ecrire du texte markdown et non une phrase) me semble suffisament pertinent pour servir de base par defaut.
Je serai donc pour le laisser. Le danger est faible et la source facilement identifiable si l'on veut changer le comportement dans des cas specifiques (et reduire le scope s'il s'avere moins generique que prevu).
Ah effectivement, il faudrait uniformiser ca. Tant que le code est clair, je pense que ce sera rapide, ca pourra être combiné à l'uniformisation vers Typescript. J'ai fait un ticket : #296 |
Il y a un problème de labels vides dans la liste de datasets de https://deploy-preview-223--ecospheres.netlify.app/bouquets/suivi-de-la-pollution-de-leau : |
La description du dataset est manquante une fois le bouquet créé (on devrait avoir le "boum" figurant dans mes commentaires précédents) : https://deploy-preview-223--ecospheres.netlify.app/bouquets/test-1 |
Fixes #144
Supersedes #145
Decomposition du formulaire de creation/edition de bouquets
Éléments supplémentaires de ma proposition
Comment tester, comment reviewer, etc. (optionnel).