Groundworks Code Review 15/06

[martingaston]

Morning all! Congratulations on getting so much done so far :tada: This looks like a really meaty project with plenty of action, so I'm sure you're all mega excited to get stuck into the second week of your build sprint.

I've tried to separate out my feedback into sections, and some of my comments are opinionated which means you shouldn't feel bad at all if you disagree. Go your own way! Have those opinions! This is your project :tada: But I thought it might be useful to throw in some more conceptual discussion - maybe as something to refer back to down the line :+1: My intention is absolutely not to try and overwhelm you or suggest that everything here should even be considered to be actioned for your current sprint.

Don't forget to support each other and make time for pairing, and I'll be around for some remote pairing if you're interested throughout the week.

ALSO don't forget that you're doing great :+1:

General

• It might be worth fleshing our your README.md's as you go - these help make you look slick and awesome, and will also be a main source of information for when you hand over the project. They will serve as the main documentation for your project in the long term, such as when you try and look over your project to describe it for a job interview in a few months. Will you remember how everything works in 3 months? Because I never can :sweat_smile:
• Lots of open branches - it's always good to tidy up as you go :sparkles:
• Accessibility scores on Lighthouse are pretty good, which is great, but tabbing through the page it seems to flow alightly differently to your layout, and it might be worth taking a peek at your outlining. I would always recommend devoting some time to consider accessibility in your sprint planning when you're working on the frontend, even with a relatively compressed build period. If you haven't already, I would encourage you put your colour scheme through a contrast checker, as well, just to be safe.
• Naming conventions, look for proper capitalisation and the like: clientSignupForm -> ClientSignupForm, Ourplatform -> OurPlatform, of course the most important thing here is consistency.
• Tests! They're hard and nobody likes doing them! But I notice you don't have a test runner at all in your build - even if you're not actively going to be testing your sprint (we didn't write any tests on ours, either) I think it can be productive to stop and think how you might test for something. At least for me, when you think about making your code testable you end up having to break it down into smaller and smaller pieces, which has lots of other nice effects on the rest of your work.

Project Management

• Really good use of the Kanban board so far - but it looks like you've thought out your process. You'll be very close to the project right now, but when working with a PO it can be a really good idea to keep these things really nice and documented to help with sprint demos, planning and future thinking.
• I would recommend you huddle at the start of next week and update your user stories to help keep track of where you are, and to estimate/assign priorities to your tasks.
• Don't forget to give your groundworks-server repo some love :heart:
• It's awesome that you're adding some updates to most of your PR's, but it can be tricky for me to work out your intention - I'd say it's likely that you're probably in frequent communication as a group so you don't need to do this, but if you're ever working in a more remote setup I'd always encourage a quick sentence at the beginning (This PR adds [x] feature towards [y] user story...) to help the rest of the team.
• If you're throwing up a PR that makes a visual change, I always think it's a nice thing if you add a screenshot of what you've done. I find this has a huge impact compared to the time investment required to do it.
• I might have missed this, but are you estimating time involved in each of your tasks? I would recommend this for your own knowledge - I routinely make a terrible job of estimating (I made an absolute howler of it just this week...) but it's something you do get better at with practice :joy:

Client

Directory Structure/Repo

• I don't think (could be wrong) that you need to commit your dist folder to version control, this is what (presumably Netlify?) will produce on its own end as part of the build process. I'm a wee bit rusty on this part :sweat
• Same with .DS_Store
• There's a lot of empty files in your directory structure, which isn't necessarily bad but is probably a good way of getting yourself confused at some point in the future.
• I'd do a big search through your app and look for comments. Are you using ESLint or Prettier, or ideally both?

React

• Don't forget that React can't use class :+1: (I actually think it can but it doesn't like doing it as class is a reserved word in JS, so your console will spit out an error and ask you use className)

    <div class="FoundersComponent">
      <h2 class="FoundersComponent--header">From the Founders</h2>
      <div class="FoundersComponent--container">

CSS

• My gut feeling is there's a lot of bitty bits of CSS here, is this something you're doing manually or is there a tool you're using to split them up? If this is the kind of structure you like it might be worth exploring one of the CSS-in-JS options, as I think you're basically working to this pattern already. But if this is working for you then fantastic :100:
• If you're doing it manually and it's not working for you, I'd maybe look into putting all your CSS into one directory, and then maybe looking for any duplication in your stylesheets.

Console.logging

I'd say some of these comments can come be tidied up, and I'd take a look at L19 of ClientSignupForm.js as well - you never know who is going to stumble upon these things.

Duplication

/routes

Most of your routes here seem to be wrapping other components in a <div> - might you consider just throwing the components directly to the routes and help trim your codebase down?

LoginPortal/LoginPortal.js

<h2 className="form--el form--input__title">Password</h2>
        <input
          className="form--el input"
          type="text"
          onChange={e => setPassword({ password: e.target.value })}
/>

RegisterPortal/RegisterPortal.js

<h2 className="form--el form--input__title">Password</h2>
        <input
          className="form--el input"
          type="text"
          onChange={e => setPassword({ password: e.target.value })}
/>

This isn't such a big deal when you're working across only a few files, but what happens when you've got to change one of these? You've encapsulated LoginPortal and RegisterPortal into the same directory and they have many of the same patterns - do you have a modular Portal component screaming to break out?

Then you can start thinking even more abstract - do all your, say, Buttons, inevitably share the same code? Is there a Button component you can make? A really nice article is 'Thinking in React' - this has the benefit of making testing those individual components much easier, and then you can build your pages by composing those smaller functions together.

I suppose a question I could ask is: what is React actually bringing to your app? Could this all (theoretically - I am in no way suggesting a pivot) have been done by Express & Handlebars templates on the backend? It might be useful for your own understanding to maybe spend a couple of minutes noodling on what the pros and cons of both options would have been.

"We wanted to do React because React is cool" is a fine answer here, mind, and that's exactly what we did for our final project as well. Because React IS cool. BUT STILL. Something to think about!

What I do like here is a line coined by Sandi Metz - duplication is cheaper than the wrong abstraction, and I would 100% of the time rather see a bunch of duplicated code than an unnatural abstraction. But maybe you are at the point where it might be prudent to start considering where you can abstract? This might help you significantly cut down on your workload, and help you design/optimise faster. Plus, when you're getting into a groove, you'll find you're probably deleting code left and right and, boy, what's more satisfying than deleting a whole bunch of code? NOTHING.

API Calls

components/clientSignupForm.js

Your API calls are coupled tightly to Axios and its API. What if you want to ditch Axios? What if your own API changes? You've probably got to go through all of your components, scattered over the place, and update everything file by file. Your component is bound to this implementation. It's probably also hard to work on this in a group, because you've got to make many changes to one file, or (this always feels worse to me) you've got to make one change to many files.

One thing you could do is extract all your API logic away from your components - let's say you make an GroundWorksAPI function. You could import this into your files that need to make API calls, and your function names can now be more tailored to what you're trying to achieve - so you could have GroundWorksAPI.registerClient() or GroundWorksAPI.getClientJob() and the like.

This also creates some nice seams and a common language in your project, so when you're pairing, one group can work on the presentational components (which don't need to worry about the business logic) and the other can plumb in all the API calls (which don't give a toot about how to actually render that information).

Server

Where does this live?

Is this going up on Heroku? I can't see a procfile. Do you have a working deployment already? I only say this because someone (me) has only narrowly dodged a few hairy deadlines from time to time (literally every time I ever deploy anything) because they (me) don't estimate well on how long it actually takes to get these kind of things up and running.

If you've already smashed this, great.

Tight Coupling

controllers/clientControllers.js

• Perhaps a little outside of the scope of a MVP, but imagine you're in production, everything is working great - working too great, and you've expanded past the limits of MongoDB. You're going to switch to Postgres. Awesome. You've got very tightly coupled code here, which has the downside of being brittle. Again, I wouldn't say this is an immediate focus but it might be worth looking up the Adapter pattern - essentially creating a middleman between your external dependencies (Mongoose, MongoDB) and your internal app.
• This also lets you code by intention and wrap all your more complicated API code inside your adapter.

Sanitising Data / Validating

• I'm not 100% sure on how Mongoose works - I'll be honest and admit that I've never touched MongoDB at all - but is this data being santized/checked at all? What happens if a req is made that doesn't have the required information?

Concurrently devStart relies on another repo

• You've got your work spread over two repos (cool) but your build script implies that you'll have access to both when you're working. And that the directory structure is exactly in the way you've got it setup, which it isn't if you just git clone down the projects by default. If you're going to separate your repos (which is cool) I'd enforce the separation in your build scripts.

[misterrodger]

Thank you, Martin! :fireworks: Yes, it is an exciting project for us and we are looking forward to next week. Thank you for your comments which are super helpful! Briefly:

• Readme - will get on it :eyeglasses:

• Branches - also on it

• Accessiblity & Layout (pages/tabbing) - good call, we have been absorbed in building components and database and haven't zoomed out focus in a week. Will do.

• Naming - We tried to set this up in our Figma but the names have drifted, will get back to the plan as you suggest, thank you.

• Tests - Yes you're absolutely right, we have neglected this and will try to get Travis and Jest running and write some tests, if that sounds good.

• Project Management/Pull Requests - thank you, all sounds good and we will incorporate.

Client

• Directory structure/repo: thank you, will gitignore dist and ds_store

• ESlint: I just made the config for separate repos last night and will make the pull request Monday morning.

• CSS: To be honest we have pushed this to the side a bit, so as you say it is just bits of CSS floating around. Honestly we have had a bit of "analysis paralysis" and ended up doing nothing...trying to decide between Sass, CSS-in-JS, or Styled Components...

:question: :question: :question: Given that we have about 3-4 days next week to complete building the MVP, which still needs authentication/sessions and the main dashboard/jobs matching page (a simple, single match at a time, so not massively complicated), would you suggest any particular CSS method above over the others? I'm happy to spend time learning over the weekend if it would be helpful, but also mindful that time is running short now.

• L19 ClientSignupForm - Wut...who wrote that? Very sorry. Everyone, remember potential employers and anybody else can see everything.

• Duplication /routes: (routes seem to be just wrapping other components in a div) I think these are all kind of placeholders at the moment that we were planning to build out, so we were keeping them modular like this, but will look to see if we can refactor some/all as you suggest.

• React & API calls - We wanted to use React in this project because the "dashboard" section seemed like a good use of React, with multiple profile cards on the same page to drop-down or toggle, and data to render quickly, and we thought it would be a good thing to handover for future use to the client. Other than that I think we also simply wanted to learn more React. You've given us some great guidance for next steps in learning and building which we will follow, as well as abstracting API calls as you suggest.

Server

Yes, server is on Heroku and Client is on Netlify currently. Front end with Webpack on Heroku as well was the original plan, but deployment seemed to be taking a lot of time to iron out the kinks, so Ryan suggested Netlify for that part and it was working last time we checked. However we have added a lot to the project since the initial deployment, so we will check again on Monday that it is still working.

Will look into the Adapter pattern as you suggest, thank you.

• Sanitising/Validating: Yes, the default is that data marked for example: "firstname: String, required: lowercase" in the MongoDB schema must be submitted that way, or the entire request is rejected with an error message. There are many options for this kind of thing that can be configured, and we've actually turned it off for the moment to ease early development.

• Mongoose for our purposes is simply to provide a thin layer that makes the connection syntax simple, and provides about 8-10 standard DB queries like "save()" or "findByID()". For our purposes it was a quick way to get up and running, and we can always use the Mongo query syntax if we need a more specific query, which is fairly close to JS. It also seemed like Mongo/Mongoose would also be a user-friendly technology to hand over to the client, relatively fast to develop and easy for them to carry forward, and we mentioned that at some point they may need to migrate.

• Concurrently - yes, we initially had both parts in one repo and then separated them when we had the challenge deploying front end to Heroku. Well-spotted, and will delete now, thank you.

Thanks again, brilliant and lots for us to work on! Cheers! :champagne:

[martingaston]

CSS: To be honest we have pushed this to the side a bit, so as you say it is just bits of CSS floating around. Honestly we have had a bit of "analysis paralysis" and ended up doing nothing...trying to decide between Sass, CSS-in-JS, or Styled Components...

I get that, Analysis Paralysis will be the subtitle of my biography 😂

I think you're absolutely right to bring up time - that's always, every time, going to be what gets you. I think Styled Components is neato, has super good documentation and is a logical fit for the structure you've already got in place, and at a complete gut feeling it might be easier to pick up compared to SASS as you're already rockin' React syntax and concepts. BUT... it also has downsides, like you say it's going to take some time to spin up and incorporate it, and refactoring it across your entire codebase will take big time.

Advantages of SASS are that it's more established, your current CSS will probably more easily get batched over to it, and it's probably more straightforward to refactor your current CSS into SCSS as you already have a neat Webpack script (also, I forgot to say, you wrote your own Webpack script? Daaaaaaaaaaaaaaang that's neat 😎)

Is it worth pulling yourselves off other user stories to work on that? What are the risks of implementing (my usual two biggies are: 1) it takes longer to skill up on tool [x] than I was anticipating, and 2) I accidentally break something along the way)

On the other side - if the solution you've got now is working... then it's working! There's a lot to be said for not potentially breaking something that's not broken

It's all about the trade-offs. There might be a 'meet in the middle' solution where you scope out testing implementing Styled Components in something you build this week (say, a button or something) and see what it's like. If you're not getting anywhere other than capital F Frustrated after a couple of hours... consider it a spike and go back. Or if you all take to it and think it's the answer to all of your CSS problems you can start rolling it out on more components.

Feel free to reach out with any other Q's 👍