-
Notifications
You must be signed in to change notification settings - Fork 1
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: new timetable page #12
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oublie pas de créer tes PRs sur l'api, je ne peux les deviner et les review sinon 😭
</Button> | ||
</div> | ||
<div className={styles.rangeLength}> | ||
<Button noStyle onClick={() => setNumberOfDays(1)}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Le style est ptet à revoir je pense 😅
D'ailleiurs puisque je suis au niveau des boutons, ce serait nice de mettre un bouton pour revenir au jour actuel
src/app/timetable/page.tsx
Outdated
</Button> | ||
<p> | ||
{format(firstDay, `d MMMM yyyy`, { | ||
locale: locale.fr, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ça donnerait quoi si on traduit le site dans sa version anglaise ? Y a moyen de gérer ça ici ? (pareil pour les 2 autres en dessous)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En effet, j'avais dû le mettre parce que mon PC est en anglais et du coup j'avais la version anglaise du texte par défaut, tu peux vérifier de ton côté que ça affiche bien dans la langue du PC et pas en anglais par défaut ?
<div className={styles.settings}> | ||
<div className={styles.range}> | ||
<Button noStyle onClick={() => setFirstDay(new Date(firstDay.getTime() - DAY_LENGTH * numberOfDays))}> | ||
{'<'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
En vrai pourquoi ne pas utiliser une icône LeftChevron ? (idem symétriquement pour RightChevron)
const api = useAPI(); | ||
useEffect(() => { | ||
const offset = numberOfDays === 7 ? -(firstDay.getDay() === 0 ? 6 : firstDay.getDay() - 1) : 0; | ||
setFirstDay(new Date(firstDay.getTime() + offset * DAY_LENGTH)); // I verified, there is no problem with changing the clocks :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logique que ça change pas si tu changes l'horloge (puisqu'on travaille avec des timestamps - donc relatifs au début de l'époque Unix en UTC)
Le changement de fuseau horaire pourrait poser des soucis mais pas à cet endroit là : imagine tu regardes un emploi du temps avec des heures françaises alors que t'es sur un autre fuseau horaire, qu'est ce que ça donne ?
En l'occurrence (j'ai pas vérifié, je dis ça en lisant le code), vu que firstDay
est défini par rapport au fuseau horaire de l'utilisateur, les évènements des emplois du temps vont se décaler au niveau des jours et des heures en fonction du décalage horaire pour correspondre aux jours et heures du fuseau horaire du pc
Je ne sais pas si ce qui est pertinent entre le fait de s'adapter au fuseau horaire local ou de proposer un edt "fixe" :
- d'une manière si t'es à l'étranger et que tu regardes l'edt de quelqu'un ça peut être sympa de voir directement "ah le lundi t'as SY06, IF36, LO02 et HT44" et pas "Wait y a SY06 et IF36 le dimanche soir et LO02 et HT44 tôt le matin le lundi"
- après si tu veux voir quand tu peux appeler la personne par exemple, ajuster les horaires en fonction du fuseau horaire local est plus que pertinent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
De mémoire j'avais eu un ptit traumatisme avec les fuseaux horaires dans cette PR >.<
Je pense que ce point ça peut être un truc pour plus tard, je pense que c'est le genre de détail sur lequel faut pas forcément s'attarder sinon on aura jamais fini (on pourra toujours faire mieux). Par contre ouais, en vrai ça peut être sympa de tenir une liste. Flemme de le faire ce soir tho, je le fais demain si j'y pense
const [eventIndicesPerDay, setEventIndicesPerDay] = useState([] as number[][]); | ||
const api = useAPI(); | ||
useEffect(() => { | ||
const offset = numberOfDays === 7 ? -(firstDay.getDay() === 0 ? 6 : firstDay.getDay() - 1) : 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peut être plus simple à écrire comme ça ?
const offset = numberOfDays === 7 ? -(firstDay.getDay() === 0 ? 6 : firstDay.getDay() - 1) : 0; | |
const offset = numberOfDays === 7 ? -((firstDay.getDay() + 5) % 6) : 0; |
Ps : peut être commenter et préciser que le hook sert à se recaler sur le début d'une semaine ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je sais pas si c'est plus lisible tho
No description provided.