Feat: new labextension - #385
Conversation
natkam
left a comment
natkam
left a comment
There was a problem hiding this comment.
I've had a look at the hooks and left a few questions/suggestions. Maybe some of them will be relevant (;
| const diffTime = Math.abs( | ||
| props.endDate.getTime() - props.startDate.getTime() | ||
| ); | ||
| return Math.ceil(diffTime / (1000 * 60 * 60 * 24)); |
There was a problem hiding this comment.
I'm not sure if this matters (I haven't yet found where this function is used, sorry), but is it supposed to return the difference in days between two dates (e.g.: from today to tomorrow, it'd be 1 day; from Mon. to Fri., it'd be 4 days), or two times (e.g. from today midday to tomorrow 1 p.m., it'd be 2 days)? Right now, it does the latter.
Just making sure that this is what you intended :)
There was a problem hiding this comment.
I actually wanted to update it so it shows the difference in hours and minutes as well, so I will definitely change this!
| await deleteAssignment(variables.lectureId, variables.assignmentId); | ||
| }, | ||
| onSuccess: async (data, variables) => { | ||
| await queryClient.invalidateQueries({ |
There was a problem hiding this comment.
I read on the internet that await-ing invalidateQueries is not necessarily good, because it actually blocks UI updates. (But tbh, I'm not sure if our use case is the same as in the article - maybe you know better that we want to await actually?)
(I think queryClient.invalidateQueries is await-ed everywhere in the hooks, so this comment also applies to other occurrences.)
There was a problem hiding this comment.
Good point, the question is what we actually want. If we remove async, then UI will update a bit inconsistently, since for example the dialog will close and only then the new data is fetched and the UI is updated. But this happens already anyway since we omitted mutateAsync. So we either go back to mutateAsync and have more verbose code, or we keep this and allow for a small time-lag on UI updates. What do you think?
There was a problem hiding this comment.
Now, for questions about UX I'm definitely out of my depth haha! I'd have to see how it looks in reality, I think. Not sure if that small lag is even worth bothering in our case...
natkam
left a comment
natkam
left a comment
There was a problem hiding this comment.
A few more minor things that caught my eye.
| @@ -6,20 +6,21 @@ | |||
| "esModuleInterop": true, | |||
| "incremental": true, | |||
| "jsx": "react", | |||
| "lib": ["DOM", "ES2020"], | |||
| "lib": ["DOM", "ES2019", "ES2020.Intl"], | |||
| "module": "esnext", | |||
| "moduleResolution": "node", | |||
There was a problem hiding this comment.
If I understood it correctly, "node" is for old version of Node, and we probably want "node16"/"nodenext" or "bundler".
| "noUnusedLocals": true, | ||
| "preserveWatchOutput": true, | ||
| "resolveJsonModule": true, | ||
| "outDir": "lib", | ||
| "rootDir": "src", | ||
| "strict": true, | ||
| "strictNullChecks": false, | ||
| "skipLibCheck": true, | ||
| "target": "ES2020", |
There was a problem hiding this comment.
This seems inconsistent, here we have target "ES2020", but earlier we set "lib": ["DOM", "ES2019", "ES2020.Intl"].
Co-authored-by: Natalia Maniakowska <3858688+natkam@users.noreply.github.com>
natkam
left a comment
natkam
left a comment
There was a problem hiding this comment.
okay, then let's merge this one, and I'll next have a look at the other PR :)
2 participants
Code migration from GitLab.