Skip to content

Kevin jayat#1

Open
KevinJayke wants to merge 8 commits intotymate:mainfrom
KevinJayke:Kevin-Jayat
Open

Kevin jayat#1
KevinJayke wants to merge 8 commits intotymate:mainfrom
KevinJayke:Kevin-Jayat

Conversation

@KevinJayke
Copy link

Bonjour @tymate

Comme convenu je vous envoie le code à trous de Weather app.
Il manque la fonctionnalité du dernier sprint, mais comme elle me semblait complexe, je ne voulais pas l'expédier.

Vous avez bien insisté sur le fait de ne pas trop y passer du temps, mais je peux néanmoins travailler sur cette dernière dès Lundi, ça sera un excellent entraînement pour moi.

Merci pour ce challenge, j'ai hâte d'avoir vos retours !

Bon week-end tout le monde.

Kévin

@netlify
Copy link

netlify bot commented Mar 18, 2022

✔️ Deploy Preview for tymate-interview-weather ready!

🔨 Explore the source changes: c182ba7

🔍 Inspect the deploy log: https://app.netlify.com/sites/tymate-interview-weather/deploys/6234b98eff87460009c66916

😎 Browse the preview: https://deploy-preview-1--tymate-interview-weather.netlify.app/

@Uptip Uptip requested review from Uptip and cynthiahenaff March 19, 2022 08:41
Comment on lines 12 to +19
templateColumns="minmax(15em, 1fr) 3fr"
>
sx={{
'@media (max-width: 450px)': {
display: 'flex',
flexDirection: 'column'
},
}}
>
Copy link
Contributor

Choose a reason for hiding this comment

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

Ça fonctionne. Comme on a un Grid au-dessus, on peut aussi utiliser une prop responsive sur le container, pour qu’il n’y ait 2 colonnes dans la grille qu’à partir d’un certain endpoint.

templateColumns={{ md: 'minmax(15em, 1fr) 3fr' }}

Comment on lines +5 to +37
return windDirection == 'N' // direction
? 0 // degree
: windDirection == 'NNE'
? 30
: windDirection == 'NE'
? 50
: windDirection == 'ENE'
? 70
: windDirection == 'E'
? 90
: windDirection == 'ESE'
? 120
: windDirection == 'SE'
? 140
: windDirection == 'SSE'
? 160
: windDirection == 'S'
? 180
: windDirection == 'SSW'
? 210
: windDirection == 'SW'
? 230
: windDirection == 'WSW'
? 250
: windDirection == 'W'
? 270
: windDirection == 'WNW'
? 300
: windDirection == 'NW'
? 320
: windDirection == 'NNW'
? 340
: 0; // goes to 0° if the value is undefined
Copy link
Contributor

@Uptip Uptip Mar 25, 2022

Choose a reason for hiding this comment

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

Ici j’ai 3 remarques,

  1. On a, sur today, la windDirection en degrés, plus précis qu’en points cardinaux
  2. La direction du vent, c’est là d’où il vient, mais la flèche représente souvent là où il va, du coup si on utilise les points cardinaux, S c’est vers le haut, E vers la gauche, W vers la droite, etc. (du coup si utilise today?. windDirection, il suffit d’ajouter 180)
  3. Pour une longue ternaire comme ça, n’hésite pas à utiliser des switch case, plus lisibles
    e.g.
export const angle = windDirection => {
  switch (windDirection) {
    case 'N':
      return 180;
    case 'NNE':
      return 202.5;
    case 'NE': 
      return 225;
    // […]
    case 'S':
    default:
      return 0;
  }
}


export function formatDate(date) {
try {
return format(new Date(date), 'eee d MMM');
Copy link
Contributor

Choose a reason for hiding this comment

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

🙌

Comment on lines +22 to +26
<Select placeholder="Choose your city" variant="flushed">
<option value="option1">Lille</option>
<option value="option2">Lyon</option>
<option value="option3">Bordeaux</option>
</Select>
Copy link
Contributor

Choose a reason for hiding this comment

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

Du coup pour ça, dans AppProvider on a une constante const CITIES,

Tu peux l’exposer dans ton contexte, genre

<AppContext.Provider value={{ 
  availableCities: CITIES, 
  selectedCity, 
  onSelectCity: setSelectedCity 
}}>

Dans Sidebar.jsx,

const { availableCities } = useApp();
// [...]
<Select
  placeholder="Choose your city"
  variant="flushed"
  onChange={e => handleSelectCity(e.target.value)}
>
  {availableCities.map(({ id, label }) => (
    <option key={id} value={id}>
      {label}
    </option>
  ))}
</Select>

Dans useWeather.js

import { useQuery } from 'react-query';
import api from '../api';
import useApp from './useApp';

const useWeather = () => {
  const { selectedCity } = useApp();
  // const locationId = 608105;

  const response = useQuery(
    ['weather', selectedCity],
    () => api.get(`/location/${selectedCity}/`),
    {
      enabled: Boolean(selectedCity),
    },
  );

  return response;
};

export default useWeather;

Quelque chose comme ça. 🎉

Copy link
Author

Choose a reason for hiding this comment

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

Wow merci pour tous ces retours très constructifs et détaillés ! Je m'entrainerai à les appliquer sur l'appli 💯

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