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

[GEOLOC AUTOCOMPLETION] React component and API routes #273

Closed
wants to merge 9 commits into from

Conversation

ArthurLeblanc
Copy link

Add :

  • React hook that delivers geolocation autocomplete in all languages

  • Places details & translation service

  • Story book component for demo purpose

Example of the story book component with translation set to russian :

brave_2020-05-15_12-22-18

@Clm-Roig
Copy link
Member

Clm-Roig commented May 18, 2020

Ça m'a l'air bien dans l'ensemble.
Je vais pull sur ma machine d'ici demain au plus tard pour vérifier en détails.

Juste une remarque : faudrait bouger le code d'appel aux API externes dans des actions puis des reducers (flux de données Redux déjà en vigueur sur Grottocenter). Peut-être que ça peut être fait plus tard cependant.

@Clm-Roig
Copy link
Member

Clm-Roig commented May 18, 2020

Faudrait passer un coup de linter avant de merger et corriger toutes les erreurs et warnings 😉 (supprimer les console.log, mettre un id aux li, éviter les commentaires trop longs...)

image

@Clm-Roig
Copy link
Member

Clm-Roig commented May 18, 2020

Il manque react-select dans le package.json.
Pensez à le sauver avec npm install react-select -s

EDIT : il n'est pas utilisé en fait non si je ne dis pas de bêtises ?

@ArthurLeblanc
Copy link
Author

Il manque react-select dans le package.json. Pensez à le sauver avec npm install react-select -s

Pour le coup on n'utilise pas react-select finalement, mais j'ai effectivement oublié d'enlever le import ! Il faut que je corrige ça.

Comment on lines 3 to 4
/* global google */
const API_KEY = '';
Copy link
Member

Choose a reason for hiding this comment

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

Le global google c'est pas très propre... On doit en faire quoi concrètement ? D'où il sort ? 🙂

Il faudrait mettre en place un système de variable d'environnement pour la clé d'API : je ne crois pas qu'il y en ait sur l'application React pour le moment...

Copy link
Contributor

Choose a reason for hiding this comment

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

@Clm-Roig J'avais pas vu ton message mais je viens de dire la même chose sur Slack
Oui il faudrait
On à aucune autre global dans le front ? (Pour leaflet ou un autre service)
Je n'ai pas l'impression

assets/js/react/hooks/useLocationPredictions.js Outdated Show resolved Hide resolved
assets/js/react/hooks/useLocationPredictions.js Outdated Show resolved Hide resolved
assets/js/react/hooks/useLocationPredictions.js Outdated Show resolved Hide resolved
assets/js/react/hooks/useLocationPredictions.js Outdated Show resolved Hide resolved
assets/js/react/hooks/useLocationPredictions.js Outdated Show resolved Hide resolved
const predictionsCity = useLocationPredictions(inputCity, 'city', undefined, countyBounds, 3);

// Triggered when an option is selected, changes the state and retrieves detail of the selected option to get bounds information.
const onSelect = (selected, type) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

selected c'est une prediction non ?
Autant mettre le même terme qu'en bas. Ou alors renommer en selection
selected ça fait penser à un boolean

assets/js/react/hooks/useLocationPredictions.js Outdated Show resolved Hide resolved
@maximenathan
Copy link
Contributor

maximenathan commented May 20, 2020

Je n'arrive pas à tester le composant dans storybook
J'ai des erreurs dans la console

image

Merci @ArthurLeblanc
Il faudrait donc rajouter le script js dans le fichier de config storybook

<script
type="text/javascript"
src=" https://maps.googleapis.com/maps/api/js?key=YOUR_API_KEY&libraries=places "></s
cript>

Et rajouter une api google dev pour grotto non ? @Clm-Roig si tu as une idée :)

@maximenathan
Copy link
Contributor

Faudrait passer un coup de linter avant de merger et corriger toutes les erreurs et warnings (supprimer les console.log, mettre un id aux li, éviter les commentaires trop longs...)

Vous utilisez quoi comme IDE ?
J'imagine que vous n'avez pas configuré votre linter pour qu'il utilise la config du projet
Ou alors vous utilisez Emacs et c'est suicidaire :p

Copy link
Contributor

@maximenathan maximenathan left a comment

Choose a reason for hiding this comment

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

Modulo #273 (comment)
Pour moi on peut merge et refaire un test fonctionnel après

Le code est bien

Comment on lines +510 to +516
cors: {
allowOrigins: [
'http://beta.grottocenter.org/',
'http://beta2.grottocenter.org/',
'https://www.grottocenter.org/',
],
},
Copy link
Member

Choose a reason for hiding this comment

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

A tester mais je pense que ce n'est plus nécessaire maintenant qu'on a séparé le front du back.

Comment on lines +522 to +529
cors: {
allowOrigins: [
'http://beta.grottocenter.org/',
'http://beta2.grottocenter.org/',
'https://www.grottocenter.org/',
],
},
},
Copy link
Member

Choose a reason for hiding this comment

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

A tester mais je pense que ce n'est plus nécessaire maintenant qu'on a séparé le front du back.

@Clm-Roig Clm-Roig changed the title Feature/geoloc autocompletion [GEOLOC AUTOCOMPLETION] React component and API routes Dec 18, 2022
@urien
Copy link
Contributor

urien commented Mar 25, 2023

Closed by #1160

@urien urien closed this Mar 25, 2023
@Clm-Roig Clm-Roig deleted the feature/geoloc_autocompletion branch July 18, 2023 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants