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: 🎸 je consulte le détail d'un article (v1) #77

Merged
merged 2 commits into from
Mar 30, 2021

Conversation

benguedj
Copy link
Contributor

Closes: #28

@benguedj benguedj requested a review from Alezco March 29, 2021 11:40
action: () => void;
icon?: React.ReactElement;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
icon?: React.ReactElement;
icon?: React.ReactNode;

Copy link
Contributor

Choose a reason for hiding this comment

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

Peut-être que tu pourras retirer le ? avec ReactNode puisque le type comprend undefined il me semble

@@ -25,7 +26,7 @@
"expo": "~40.0.1",
"expo-asset": "~8.2.2",
"expo-constants": "~10.0.1",
"expo-font": "~9.0.0",
"expo-font": "^8.4.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

Pourquoi un downgrade ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai downgrade la version car la v9 ne fonctionne pas bien avec les fonts custom, j'ai commencé à créer une font d'icônes ça sera beaucoup plus simple à gérer pour changer la couleur et importer chaque icône à chaque fois. Je sais pas si tu connais icomoon c'est un outil en ligne qui permet de générer des fonts à partir de svg (https://icomoon.io/app/#/select)


if (loading) {
return (
<View>
Copy link
Contributor

Choose a reason for hiding this comment

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

A isoler dans son propre composant ? On risque d'en avoir besoin à nouveau dans d'autres fichiers

<ActivityIndicator size="large" />
</View>
);
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Les else ne sont probablement pas nécessaires. Je pense que loading, error et data sont 3 états différents qui ne peuvent pas valoir quelque chose en même temps. Je pense que le code peut être simplifié comme dans cet exemple : https://www.apollographql.com/docs/react/data/queries/#executing-a-query

<View style={[styles.mainContainer]}>
<View>
<View style={[styles.flexStart]}>
<Button
Copy link
Contributor

Choose a reason for hiding this comment

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

Ce bouton retour est peut-être aussi à isoler dans son propre composant puisqu'il sera sûrement utile ailleurs à l'avenir

</ListItem.Content>
</ListItem>
))}
<ActivityIndicator size="large" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Même commentaire ici pour le composant de Loading à sortir de ce fichier (peut-être aussi celui d'erreur), et aussi sur les if else

);
} else {
if (error) {
console.log(error);
Copy link
Contributor

Choose a reason for hiding this comment

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

A retirer ?

{result.articles.length} article(s) à lire
</Text>
{result.articles.map((article, index) => {
return (
Copy link
Contributor

Choose a reason for hiding this comment

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

L'écriture peut être simplifiée en retirant le return et les accolades

{article.titre}
</ListItem.Title>
<ListItem.Subtitle style={[styles.articleDescription]}>
<HTML
Copy link
Contributor

Choose a reason for hiding this comment

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

A changer peut-être à l'avenir. Je n'aime pas beaucoup l'idée d'avoir une webview pour la description sur chaque élément de la liste. L'idéal serait peut-être d'avoir depuis le back-end une description ou un résumé de l'article sous forme de string et pas de HTML pour simplifier le code ici. @risseraka Qu'en penses-tu ?

Copy link
Contributor

Choose a reason for hiding this comment

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

C'est à configurer.
S'il n'y a pas de besoin d'éditorialiser le résumé, alors on peut s'en tirer avec un champ de type description.
À valider avec le métier ?

benguedj pushed a commit to benguedj/1000jours that referenced this pull request Mar 30, 2021
@benguedj benguedj merged commit 1372e19 into SocialGouv:master Mar 30, 2021
SocialGroovyBot added a commit that referenced this pull request Mar 31, 2021
# [1.5.0](v1.4.3...v1.5.0) (2021-03-31)

### Bug Fixes

* **android:** correction de problèmes d'affichage sur Android ([#80](#80)) ([5a4c17e](5a4c17e))
* **api:** ajoute la variable d'environnement de non-authentification pour le déploiement ([#75](#75)) ([c4e5cea](c4e5cea))
* **backoffice:** corrige l'affichage des champs textes HTML ([#78](#78)) ([2d1a40a](2d1a40a))
* **k8s:** another PVC fix ([#84](#84)) ([d9e240a](d9e240a))
* **k8s:** fix PVC issue with kube 1.19 ([#79](#79)) ([cab3556](cab3556))

### Features

* 🎸 je consulte le détail d'un article (v1) ([#77](#77)) ([1372e19](1372e19)), closes [#28](#28)
@SocialGroovyBot
Copy link
Member

🎉 This PR is included in version 1.5.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ETQ utilisateur, je consulte le détail d'un article (v1)
4 participants