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

Project mentor - Code review #66

Open
renatadev opened this issue Jul 6, 2023 · 0 comments
Open

Project mentor - Code review #66

renatadev opened this issue Jul 6, 2023 · 0 comments

Comments

@renatadev
Copy link

Code

  • It is worth mentioning somewhere (perhaps README) that the project must run on port 3000 (I failed at the first hurdle to run the project, as I was running it on another port) so the callback auth works or, even better, you should build the path from the URL instead of fixing it to localhost (which I imagine you'll be doing quite soon). See code below:
    redirectTo="http://localhost:3000/auth/callback"
  • Error pages:
    I tried clicking in an old invite link and I still was able to see the page rendering as usual, however the URL did demonstrate an error should have occurred. Would be good to add some better error handling around this. You could map the error codes to display certain error message or just build a generic error page that returns you to either the magic link or home page if not logged in?

Example of URL:

http://localhost:3000/project#error=unauthorized_client&error_code=401&error_description=Email+link+is+invalid+or+has+expired

UI / UX

  • Consider using text-overflow: ellipsis CSS property on the titles of the tasks. There could be places where the user cannot tell if there’s more content. This may also apply to other inputs across the page.
Screenshot 2023-07-06 at 18 35 25
  • Consider using cursor pointers on the checkboxes for better UX
  • UX wise, would be good to add something else other than the checkboxes to inform the user that a task has been completed (I know you mentioned confetti, etc) but there may be something extra you can do to highlight this, for example, greying out the tasks when completed to give less prominence to it. I think that may be your intention by the look of the Figma file, but you may want to enhance those colours a little bit more as it isn’t quite serving the purpose at the moment.
  • Double checkboxes on main sub task (!) are a bit confusing at the moment as it has the same logic as the other one. However checkboxes are behaving pretty nicely. Well done!!
Screenshot 2023-07-06 at 18 49 51
  • Minimal thing but I think that adding better icons like the ones in your figma files will help enhance the design and help the user navigate through such small icons. Theres infinite ways of doing this, you can either export the icons from your figma files, or use a icons library like google’s one
  • It will be nice for the next iteration to integrate the functionality of the expand and add tasks as at the moment the icons are quite confusing (I admit this is only the case because they actions on those icons haven’t yet been enabled
  • Something a bit funky is going on with the arrows when closing a task. (Just raising it up in case you haven’t noticed it and bring it to your attention… you probably are on it already!)
Screenshot 2023-07-06 at 18 51 24

Project structure

Project structure looks god. I like you’ve got dedicated folders for documentation (v nice, I have never done this!), utils (these are usually really good pieces of logic that you could test, although atm you only have one fx there), …

The only thing about the project structure is that it would have helped me (this is personal, but might help some of you too) to group the components within the pages and put all the pages in a folder.

pages >
   project >
	components >
	   Title
           Task
	page.tsx
	acount >
	page.tsx

I am also confused as why the account-form.tsx isn’t the same casing as the other components? Isn’t this a component too? Same for Styles - is this a special case?

The best parts <3

  • FE is looking really nice 😍
  • I of course, love that you’re using Typescript <3 <3 <3
  • I like the addition of the magic link - makes it feel special and intriguing to see what’s behind that first page. Also good way of getting started with user / login methods (I noticed the sign out and the account page in progress! great stuff)
  • Fantastic work as a team, contributions are looking pretty nice and almost consistent, looks like you’re having a good time. You've taken a big challenge and you've already achieved a lot! Well done all! Keep smashing it!

Recommendations

(can ignore if this is all too much to read)

  • If you wanted to make some of your fields in your types optional, you can add a ? to the type, for example, let’s say you don’t always require a description in your Issues, you can just do the following:
export interface Issue {
  id: number
  taskId: number
  title: string
  description?: string // add a ? and so when you use this type, it won't yell at you if a description isn't defined
  timeEstimate: string
  done: boolean
}
  • Noticed you’re using husky, you could also use it to enforce some commit messages format so all your commits follow the same conventions ;). It can be a bit annoying at first but it will force you all to keep up with it and you’ll get use to it pretty quickly.
  • I’ve successfully updated my username :-) but this page requires attention, if you do want to show it may be worth it chucking a few tailwind classes there to make it a bit nicer! Although I suggest focusing on the main page for now, try getting as much of the logic implemented and bother about accounts later on, I think that is a nice to have rather than a must have at this stage.
  • Just a recommendation for future projects: When building components, it may be really nice to add something like storybook to test your components / pages in isolation with mock data. This way you don’t have to constantly change routes / use the components in the main route to test. That would have been good here to test the implementation that you’re trying to achieve with the APIs but without the need to hook it up or even just to demonstrate that different variations of the components that you already have (i.e: tasks opened, closed, collapsed, etc). It also is very handy for documentation. This may have been an overkill for this project but it is something I thought you can consider for the future!
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

No branches or pull requests

1 participant