-
Notifications
You must be signed in to change notification settings - Fork 0
Open
Description
Hi Istvan, Niklas from Ticketmaster here! I hope you don't mind me submitting this as an issue, but after reading the code I have a couple of suggestions and some questions that I'm hoping you can answer. Thanks!
- I was not able to run the project locally, so adding step by step instructions on how to run locally in the readme would be helpful
- Labels are not tied to the input with
foror by wrapping the input field which is not good for screen reader users - Unless I'm mistaken, '/api/users/currentuser' is called twice during server side render (client/pages/_app.js and client/pages/index.js)
- It is not clear to me where
css/styles.scssis included. The bootstrap css file is included in pages/_app.js but notcss/styles.scss - I would recommend looking into component level scss (with css-modules) to make it clear what styles are related to what component and avoid class name clashes. I think it is supported in Next.js by default, see https://nextjs.org/docs/basic-features/built-in-css-support#sass-support
- .gitignore should probably include client/.next directory
- Nice use of a custom react hook for data fetching 👍
Questions
- How would you go about adding client and/or server side input validation?
- Any reason why you chose to use TypeScript for the backend services but not for the frontend?
- The code is nicely formatted, are you using something like prettier or eslint for this?
- I could not see any tests for the client code. Do you usually add tests for the frontend as well or do you feel it's better to focus on testing the backend?
Metadata
Metadata
Assignees
Labels
No labels