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

[Fix]: Affichage distance établissement #568

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Slysoks
Copy link
Contributor

@Slysoks Slysoks commented Jan 4, 2025

🚀 Nouvelle Pull Request

Checklist d'avant pull request

  • Vous avez testé de build le projet avec vos modifications et ce build a réussi
  • Vous respectez les conventions de codage et de nommage du projet
  • Vous utilisez la tabulation pour l'indentation afin de maintenir un code lisible
  • Cette pull request n'est pas un duplicata d'une autre
  • Cette pull request est prête à être revue (review) et fusionnée (merge)
  • Il n'y a pas de TODO (aka des annotations pour du code manquant) dans vos modifications
  • Il n'y a pas d'erreurs de langue dans votre code (grammaire, vocabulaire, conjugaison, orthographe)
  • Les détails des changements ont été décrits ci-dessous
  • Cette pull-request n'est pas une "breaking-change" (des modifications qui vont entraîner la modification du fonctionnement de certaines fonctionnalités déjà existantes)

Changelogs proposés

Enlèvement de l'affichage de la distance avec l'établissement lors de la sélection de la ville manuel

Screenshot_20250104_233500_Expo Go.png

Screenshot_20250104_233518_Expo Go.png

@Clmnnt
Copy link
Contributor

Clmnnt commented Jan 4, 2025

de ta position c pas mieux ?

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 4, 2025

Comment ça ?

Copy link
Contributor

@Gabriel29306 Gabriel29306 left a comment

Choose a reason for hiding this comment

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

J'aime bien le principe.
Par contre, le débug, ça ne devrait pas passer en prod (même s'il y a des écarts...).

src/views/login/pronote/PronoteInstanceSelector.tsx Outdated Show resolved Hide resolved
src/views/login/pronote/PronoteInstanceSelector.tsx Outdated Show resolved Hide resolved
@Clmnnt
Copy link
Contributor

Clmnnt commented Jan 4, 2025

bah j’aurais dit à « …. km de ta position »

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 4, 2025

bah j’aurais dit à « …. km de ta position »

Mais là on ne parle pas de sa position mais par rapport à la ville, ce qui n'a pas beaucoup de sens

@Gabriel29306
Copy link
Contributor

de ta position c pas mieux ?

Bah non 🤣 (je suis crazy).
Quand tu sélectionnes une ville, c'est pas précis.
Alors que quand tu utilises la position, c'est précis, et donc ça met "à XXkm de toi"

@JyhuKo
Copy link
Contributor

JyhuKo commented Jan 4, 2025

bah j’aurais dit à « …. km de ta position »

Mais là on ne parle pas de sa position mais par rapport à la ville, ce qui n'a pas beaucoup de sens

mais comment ca par rapport a la ville genre c est calculé a partir de quel endroit de la ville

@Slysoks Slysoks requested a review from tryon-dev as a code owner January 4, 2025 23:40
@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 4, 2025

bah j’aurais dit à « …. km de ta position »

Mais là on ne parle pas de sa position mais par rapport à la ville, ce qui n'a pas beaucoup de sens

mais comment ca par rapport a la ville genre c est calculé a partir de quel endroit de la ville

Au niveau du centre ville mais c jamais précis, ça change pr chaque ville

Copy link
Contributor

@Kgeek33 Kgeek33 left a comment

Choose a reason for hiding this comment

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

Ok parfait l'indentation et l'intégration de undefined lorsqu'on sélectionne une ville

Mais le code peut être encore plus simplifié et adapté ;)

src/views/login/pronote/PronoteInstanceSelector.tsx Outdated Show resolved Hide resolved
src/views/login/pronote/PronoteInstanceSelector.tsx Outdated Show resolved Hide resolved
src/views/login/pronote/PronoteInstanceSelector.tsx Outdated Show resolved Hide resolved
@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 4, 2025

Voilà je suis d'accord avec @Clmnnt

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

Ok je reprends mon msg. Ce que @Clmnnt veut dire et c'est la suggestion que j'ai fait dans ma review, plutôt que de dire à ... de toi, dire à ... de ta position pour que ça soit plus français quand on se localise (vu que mtn, ça affiche plus le msg quand on saisi une ville dans cette pr)

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

Supprime l'accolade et le point virgule sur le TouchableOpacity et normalement, Prettier devrait être content :)

Copy link
Contributor

@Gabriel29306 Gabriel29306 left a comment

Choose a reason for hiding this comment

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

LGTM

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 5, 2025

🙃🙃

Copy link
Contributor

@Kgeek33 Kgeek33 left a comment

Choose a reason for hiding this comment

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

Parfait, LGTM !
Et n'oublie pas de résolve les précédents commentaires de Gabriel ;)

@TinAD17tin
Copy link
Contributor

PRONOTE ne renvoie pas l'adressse tous pile ? Genre l'adresse précise

@TinAD17tin
Copy link
Contributor

Moi du côté de PRONOTE j’ai ça :
image
Ça met Nom de l'établissement, code postal et distance

@TinAD17tin
Copy link
Contributor

Et étant donné que y'en a plusieurs fois le même code postale, mais pas la même distance, j'ose imaginé que c’est parce que y'a la position précise

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

Sûrement que ça revoie l'adresse précise mais on en fait quoi après ?

@TinAD17tin
Copy link
Contributor

Bah on fait un calcul de distance directement depuis la position précise plutôt que juste le centre de la ville

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

Bah on fait un calcul de distance directement depuis la position précise plutôt que juste le centre de la ville

Mais la pr fait l'inverse justement. Quand on cherche la ville, dans cette pr, ça affiche plus la distance car on s'est pas comment s'est calculé alors que la position de l'utilisateur si

@TinAD17tin
Copy link
Contributor

Bah c’est pas ce qu'il y'a dans la capture de départ de la PR, mais ok.
J’ai pas compris pas grave

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

Bah c’est pas ce qu'il y'a dans la capture de départ de la PR, mais ok.
J’ai pas compris pas grave

C'est la 1ère capture qu'il faut regarder quand on sélectionne une ville et la 2ème quand on se localise, mais c'est pas préciser je comprends

@TinAD17tin
Copy link
Contributor

TinAD17tin commented Jan 5, 2025

Bah j’ai rien dans les deux cas moi
Même en ayant laisser la localisation par papillon en "toujours" dans mes paramètres

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

Bah j’ai rien dans les deux cas moi
Même en ayant laisser la localisation par papillon en "toujours" dans mes paramètres

Ah après j'ai pas testé j'ai regardé juste le code
Att je (re) regarde le code
Bizarre ça devrait fonctionner, j'vais tester plus tard et je te dis

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 5, 2025

Bah j’ai rien dans les deux cas moi Même en ayant laisser la localisation par papillon en "toujours" dans mes paramètres

Moi ça marche en tout cas

@TinAD17tin
Copy link
Contributor

Bah j’ai rien dans les deux cas moi Même en ayant laisser la localisation par papillon en "toujours" dans mes paramètres

Moi ça marche en tout cas

Oui mais avec la version de cette PR ou la version de base ?
Comme j’ai rien compris a la PR je disais que moi de base j’ai pas vu que on m'a dit que c'était pour l'enlever ou je ne sais pas quoi

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

@Slysoks après test, ça fonctionne bien !

@TinAD17tin regarde:

Avant :

Screenrecorder-2025-01-05-14-32-15-197.mp4

Après :

Screenrecorder-2025-01-05-14-31-47-721.mp4

@TinAD17tin
Copy link
Contributor

Ah okay.
Mais du coup pourquoi enlever la fonction c’est toujours mieux que rien non !?
Et moi du coup bah y'a pas sur ma version même en activant la localisation

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 5, 2025

Ah okay. Mais du coup pourquoi enlever la fonction c’est toujours mieux que rien non !? Et moi du coup bah y'a pas sur ma version même en activant la localisation

T sur iOS ou Android ?

@TinAD17tin
Copy link
Contributor

Ah okay. Mais du coup pourquoi enlever la fonction c’est toujours mieux que rien non !? Et moi du coup bah y'a pas sur ma version même en activant la localisation

T sur iOS ou Android ?

iOS

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 5, 2025

et toi @Kgeek33 ?

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

et toi @Kgeek33 ?

Sur Android

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

@Slysoks t'as regardé si ça fonctionne sur ios de consulter les précédentes pages ?

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 5, 2025

@Slysoks t'as regardé si ça fonctionne sur ios de consulter les précédentes pages ?

G pas d'iphone ni de mac donc je peux pas savoir sur iOS ce que ça donne, ça clc

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 5, 2025

@Slysoks t'as regardé si ça fonctionne sur ios de consulter les précédentes pages ?

G pas d'iphone ni de mac donc je peux pas savoir sur iOS ce que ça donne, ça clc

C'est ça apple 😢

Copy link
Contributor

@ecnivtwelve ecnivtwelve left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@Lukiluc29
Copy link

Je trouve que le mieux à faire de mon point de vu serait de mettre "à .. km de *nom de la ville recherché" quand on utilise "Rechercher une ville " et "à .. km de ma position" quand on utilise "Utiliser ma position".

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 5, 2025

Mais étant donné que c'est dans la ville, ça devrait être à 0km donc c pas ouf

@godetremy
Copy link
Contributor

Mais étant donné que c'est dans la ville, ça devrait être à 0km donc c pas ouf

Non car le point de ville se situe à la mairie de celle-ci

Mais bref pour moi c'est vraiment bien de voir la distance, puis ça a les autres avantages techniques pour le tri !

@TinAD17tin
Copy link
Contributor

Le calcul il est pas bon.
Je suis dans mon lycée et la (oui enfin ça m’affiche) y’a écrit je suis à 2km et quelques…

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 6, 2025

Sachez que le tri n'est pas modifié si vous aviez bien regardé le code 😉

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 6, 2025

Le calcul il est pas bon.
Je suis dans mon lycée et la (oui enfin ça m’affiche) y’a écrit je suis à 2km et quelques…

C justement ça que je fix

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 6, 2025

Eh ben on peut intégrer l'idée de @godetremy en mettant "à... de la mairie" plutôt que "à... de toi" en ce moment sur Papillon ou le vide actuellement sur la pr

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 6, 2025

Mettez pouce haut : affichage depuis la mairie Pouce bas : ne rien mettre

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 6, 2025

@Slysoks
Copy link
Contributor Author

Slysoks commented Jan 10, 2025

Prêt à merge donc

@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 10, 2025

Prêt à merge donc

ok c'est très bien comme ça !

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.

9 participants