Code review!

[charlielafosse]

Hey all 👋

First off, great job so far. It's great to see that you've got such a significant part of the site up and running so far, it looks to me like you're really nicely set up to bring all the functionality together for the final week.

I think I'll just put all my feedback here in this one issue. You can then feel free to pull out the bits (if any) you want to address into separate new issues, and ignore whatever else 😅

Git stuff

• Commit messages:
  • Looking quickly over your commit history so far, it's great to see that your commit messages are descriptive and succinct. It's easy for me to understand at a glance what's going on - so great job!
  • Something really nice I've picked up since leaving FAC is adding issue number hashes to my commit messages, linking them to the relevant issue. For example: git commit -m "#63 fix responsive design in header"
  • You could take this a step further and check out the gitmoji guide, appending meaningful emojis to your commits. eg. ♻️ As a user, I want to be able to see a list of items in my basket #12 refactor sign-up logic

Architecture

• I notice that in your components directory, components' associated .style.js files sometimes sit within the same named directory (like with Header for example), but other times are to be found within the styles directory. I'm sure there's a reason for this (is it perhaps that that styled components within styles are shared amongst different components?) but it is good practice to pick a structure and try to stick to it.

Documentation

• More stuff in README please 😁 ! The stuff you get with CRA is certainly helpful but it would've been great to have a little context about the project (who's on the team, who's the project for, what are the goals etc.) I appreciate that this is often something that comes later, but it can often be helpful (and entertaining!) to incrementally build up your documentation as you go along, using your README as a kind of log for day-to-day difficulties and breakthroughs. You're probably bored of hearing people bang on about READMEs at FAC, but I've since learnt in the world of work that they really are the front page of a project, and since employers/fellow developers will be looking there first to see things like planning, technical decisions .etc, it's a great habit to get into.

UX/UI

• Responsive Design

  • Some nice responsive bits in here already, great how your multi-column layout snaps into a single column on mobile.
  • Just be careful of things like this...
    
    At a glance, I imagine this is to do with a pesky element overflowing the body as you get down to a similar viewport width.

• Big footers

  • It might seem trivial but big footers are so hot right now. And yours is delightfully big. Love it!
  • 

• Error messages for users

  • This is really important - awesome that you've got front end checks, but in cases like these you need to make sure the user has an explanation of why their input is incorrect. I was only able to correctly format my password through inspecting the pattern attribute.
    

• on /addcontent, the Creation Date input is freeform text input at the moment - worth looking into inbuilt type attributes like date. Aside from how it looks, it also prevents users entering dates in weird and unexpected ways 👌

Accessibility

• It's good to see that you've got things like labels and aria-labels in your codebase already - it's an easy thing to not prioritise when tryin to build quickly under pressure, so great job 🙌
• V small thing - I noticed that your footer is actually built from a header styled-component: const FooterBox = styled.header. It's worth thinking about what this means for the semantics of your page, and what impact this might have on someone using assistive technologies... :thinking_face:

Tests

• HEcck! you've got tests and everything. This project is legit 🔥

JS stuff

• Not a huge amount of JS here for me to get into but I did notice your dataCall func in App.js. Great to see you getting into both promises and also async await - I don't think I had even got near touching the latter when I was doing my client project so good going! Fine to experiment but like a lot of things when we're thinking about code quality, readibility and maintability, it's a good idea to choose one of them and stick with it when you're going asynchronous.
  • In this case, it looks like you're chaining .then()s onto your await... as far as I understand it using async await is actually a way to not have to write .then(). Eg.
    fetch().then(result => //do something with result)
    vs
    let result = await fetch()
// do something with result

Linting/formatting

• Good going with the eslintrc.js. Are you guys using Prettier too? If none of you are, then ignore the following - but I've had problems in projects where different developers have different prettier configs setup in their different environments/editors. This can lead to some really annoying and pointless merge conflicts - like one person pushing code with single quotes, and someone else pulling it down, formatting it into double quotes and bunging it back up there. It's easy to get around this by using by putting a .prettierrc file along side your eslint one in the root, meaning that everyone who works on the project adheres to the same formatting rules.

---

That's all for now folks! It seems like most of the stuff you've worked on so far has been very front-end/not much JS yet, so I'm really excited to see what comes next and keen to know what the next steps are. How are you planning on building out the functionality with users being able to submit and browse content? Are you going to build your own backend, or go for an Airtable solution? Looks like you've got some exciting stuff planned with authentication and maybe even payments too?

Haven't heard a peep from you guys this week - which can often be a good sign I guess! - but please do hit me with any questions about anything small or big 🌻 What you've done so far is really impressive, so keep killing it, and make sure you rest your eyes and brains this weekend and decompress 😪 ❤️

[kin-au]

Thanks Charlie for such a great detailed review!

We did sync up our prettier settings but didn't think to have a .prettierrc file in the root - something to bear in mind in future.

We're using Netlify functions with database in Airtable so we'll be posting data to there :)

We'll make sure to start & keep updating the readme as we go, as you guessed we've just had our heads down in the code!

Again thank you for the comments, lots of really useful things for us to do and fix here