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

Test adresse comparateur #938

Draft
wants to merge 16 commits into
base: dev
Choose a base branch
from
Draft

Conversation

martinratinaud
Copy link
Collaborator

@martinratinaud martinratinaud marked this pull request as draft November 19, 2024 14:11
@martinratinaud martinratinaud force-pushed the test_adresse_comparateur branch 2 times, most recently from 3918fde to 4747571 Compare November 20, 2024 14:17
@martinratinaud martinratinaud force-pushed the test_adresse_comparateur branch from 4747571 to c3433c1 Compare November 20, 2024 14:20
@martinratinaud
Copy link
Collaborator Author

@martinratinaud martinratinaud marked this pull request as ready for review November 21, 2024 07:35
@martinratinaud
Copy link
Collaborator Author

@totakoko on peut faire une première review avant la mise en place du design

Copy link
Member

@totakoko totakoko left a comment

Choose a reason for hiding this comment

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

Top, ça charge bien ! 🏭

Quelques remarques :

  • C'est seulement pour la page d'accueil, cad pas les pages villes ?
  • Il n'y a pas de comparateur en individuel ?
  • La carte n'est pas centrée sur l'adresse (complètement dézoomé)
  • Il faut un pin sur l'adresse sur la carte
  • Ca serait mieux d'ajouter un titre à la modale non ?
  • Au chargement, 3s de non interactif, donc le chargement de la carte est bloqué
    • Ça serait bien si on pouvait déplacer ça dans un worker mais ça risque d'avoir trop d'impacts sur les hooks etc...

position: relative;
width: 100%;
`;

Copy link
Member

Choose a reason for hiding this comment

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

Tu préfères pas utiliser une Box juste pour ça ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Non j'avais pas vu, je vais d'ailleurs enlever tout le code qui a besoin de la ref (initialement utilisée pour scroller jusqu'au conteneur pour l'afficher)

renderCell: ({ value }) => value.toLocaleString('fr-FR', { currency: 'EUR', maximumFractionDigits: 0, style: 'currency' }),
},
{
headerName: 'Emissions CO2',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
headerName: 'Emissions CO2',
headerName: 'Émissions CO2',

flex: 3,
},
{
headerName: 'Cout annuel chauffage',
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
headerName: 'Cout annuel chauffage',
headerName: 'Coût annuel chauffage',

Pour harmoniser avec le reste

}, [coords, city, cityCode]);

const modesDeChauffageToDisplay = modesDeChauffage
.filter(({ type, label }) => (type as any).includes('collectif') && !label.includes('PAC'))
Copy link
Member

Choose a reason for hiding this comment

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

Normalement on ne devrait pas avoir besoin du cast car il y a que 2 choix possibles. Mais c'est plus à voir avec la définition où il faudrait ajouter un type sur type.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oui le type est mal inféré et je n'ai pas trouvé comment faire, donc je veux bien ton aide

@martinratinaud
Copy link
Collaborator Author

Top, ça charge bien ! 🏭

Quelques remarques :

  • C'est seulement pour la page d'accueil, cad pas les pages villes ?

  • Il n'y a pas de comparateur en individuel ?

  • La carte n'est pas centrée sur l'adresse (complètement dézoomé)

  • Il faut un pin sur l'adresse sur la carte

  • Ca serait mieux d'ajouter un titre à la modale non ?

  • Au chargement, 3s de non interactif, donc le chargement de la carte est bloqué

    • Ça serait bien si on pouvait déplacer ça dans un worker mais ça risque d'avoir trop d'impacts sur les hooks etc...

Merci

  • non c'est sur les pages villes aussi mais ca n'était aps sur les pages réseaux
  • non pas besoin (cf Florence)
  • le pin est ajouté
  • peut etre dans le futur design
  • Oui, je sais mais pas facile, à voir si on peut vivre avec ou si c'est rédibitoire

@martinratinaud
Copy link
Collaborator Author

J'ai intégré tous les commentaires

@martinratinaud martinratinaud marked this pull request as draft November 22, 2024 06:17
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.

2 participants