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: Handle new configuration for logos #2034

Merged
merged 1 commit into from
Nov 8, 2023

Conversation

cballevre
Copy link
Contributor

@cballevre cballevre commented Nov 8, 2023

### ✨ Features

* Handle new configuration for logos

The configuration for logos in cozy.yml will look like :


dev:
  logos:
    home:
      light:
        - src: /logos/cozy.png
          alt: Cozy Cloud
          type: main
        - src: /logos/partner1.png
          alt: Partner n°1
          type: secondary
        - src: /logos/partner2.png
          alt: Partner n°2
          type: secondary

This PR adapts to this change

Copy link
Contributor

@JF-Cozy JF-Cozy left a 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 || []
Copy link
Contributor

@JF-Cozy JF-Cozy Nov 8, 2023

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" ?

Copy link
Contributor

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).

src/components/FooterLogo/FooterLogo.jsx Show resolved Hide resolved
@@ -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'
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link

bundlemon bot commented Nov 8, 2023

BundleMon

Files updated (2)
Status Path Size Limits
vendors/home.(hash).js
1.21MB (+30B 0%) 1.5MB
app/home.(hash).js
44KB (-68B -0.15%) 50KB
Unchanged files (11)
Status Path Size Limits
services/softDeleteOrRestoreAccounts/home.js
479.68KB 490KB
services/updateAccounts/home.js
463.49KB 464KB
services/deleteAccounts/home.js
312.7KB 500KB
services/myselfFromIdenties/home.js
236.85KB 240KB
services/polyfillFetch/home.js
98.39KB 99KB
intents/home.(hash).js
33.4KB 35KB
vendors-home.(hash).(hash).min.css
30.02KB 35KB
services/attributesHelpers/home.js
15.12KB 16KB
app-home.(hash).min.css
14.1KB 15KB
intents-home.(hash).min.css
5.78KB 6KB
intents/index.html
498B 1KB

Total files change -39B 0%

Final result: ✅

View report in BundleMon website ➡️


Current branch size history | Target branch size history

@Crash--
Copy link
Contributor

Crash-- commented Nov 8, 2023

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

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 ?

@JF-Cozy
Copy link
Contributor

JF-Cozy commented Nov 8, 2023

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

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...

@cballevre cballevre merged commit 78bcb8d into master Nov 8, 2023
4 checks passed
@cballevre cballevre deleted the feat/handle-new-configuration-for-logos branch November 8, 2023 13:12
@Crash--
Copy link
Contributor

Crash-- commented Nov 8, 2023

On pourrait donc mutualiser la création et exécution de la requête

Mutualiser un useQuery(Q('io.cozy.settings').getById()) non on va pas faire ça ^^. L'affichage de l'image aujourd'hui est très dépendant de là où tu l'affiches. On va pas faire ça non plus.

Si un jour on a le besoin d'afficher de la même manière que dans coach les images pourquoi pas, en attendant :

  • la mutualisation de la requête n'apporte rien (si ce n'est de l'implicite, de ne pas savoir d'où vienne les images (quel doctype ? Est-ce que ton app a la perm dessus ? , de potentiellement ne pas avoir la même clé sur ta requête et donc de pas avoir le bénéfice du store de cozy-client)
  • le composant : pas de benefice. Au contraire la mutualisation va nous faire avoir une API super compliquée
  • On pourrait mutualiser la génération du lien vers assets/ histoire de gérer les slash qui trainent mais bon

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.

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.

3 participants