-
Notifications
You must be signed in to change notification settings - Fork 26
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: Handle new configuration for logos #2034
Conversation
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.
Peut-être que ça vaut le coup de faire un hook ou un composant dans cozy-ui car on a le même besoin sur cco2 🤔 Est-ce que tu aurais du temps pour ça ?
export const FooterLogo = () => { | ||
const client = useClient() | ||
const rootURL = client.getStackClient().uri | ||
|
||
const contextQuery = buildContextQuery() | ||
const { data } = useQuery(contextQuery.definition, contextQuery.options) | ||
|
||
const logos = getHomeLogos(data, rootURL) | ||
const logos = data?.logos?.home?.light || [] |
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 pourrait commenter ici pour indiquer que seul le theme light est supporté (car forcé). D'ailleurs, pourquoi light puisqu'on est en dark sur la home, non ? 🤔 Ca veut dire que les logos ne sont pas rangé par theme, mais par "style" ?
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.
vu en zoom, ils sont bien rangés par thème 👍 donc home.light fait référence au logo pour un thème light (donc une version foncé du logo).
@@ -3,20 +3,19 @@ import { useClient, useQuery } from 'cozy-client' | |||
import { buildContextQuery } from 'queries' | |||
import Divider from 'cozy-ui/transpiled/react/Divider' | |||
|
|||
import { getHomeLogos } from 'components/FooterLogo/helpers' |
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.
le fait d'avoir un helper permet de tester spécifiquement, cf la question sur le plantage si logos vaut un tableau vide
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.
Encore faut-il que ça vaille la peine d'être testé. Ici en l'occurrence, je suis assez d'accord avec ce qui a été fait. On peut effectivement ajouter un test sur un homeLogo vide et vérifier que rien n'est affiché. AMA pas besoin de plus.
BundleMonFiles updated (2)
Unchanged files (11)
Total files change -39B 0% Final result: ✅ View report in BundleMon website ➡️ |
Il ferait quoi le hook ou le composant ? Parce qu'on a quand même des affichages bien différents entre les apps. Et le hook ne serait qu'un useQuery puisque les histoires de secondary et de main ne sont pas présents dans CCO2. Non ? |
Il n'y pas le notion de secondary et de main dans CCO2 en effet, mais c'est la seul différence. On pourrait donc mutualiser la création et exécution de la requête, ainsi que donc la récupération des logos, puis l'affichage avec un img. On pourrait imaginer aussi ajouter la notion main/secondary spécifiquement pour la Home. Pour CCO2, on va copier 80% du code présent ici, je me disais donc que... |
Mutualiser un Si un jour on a le besoin d'afficher de la même manière que dans coach les images pourquoi pas, en attendant :
Franchement je pousse à la mutualisation, mais faisons de la mutualisation raisonnée. Il faut qu'elle apporte quelque chose. Il faut qu'elle réponde à un objectif. Ici je n'en vois pas. Il n'y a pas de mal à faire du copier / coller, bien au contraire, surtout dans ce genre de cas. On verra à la fin sur cco2 ce que ça donne, mais aujourd'hui la mutualisation me parait bien trop prématurée. |
The configuration for logos in
cozy.yml
will look like :This PR adapts to this change