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

Taha's Code Review #45

Open
Taha-Hassan-Git opened this issue Oct 26, 2023 · 0 comments
Open

Taha's Code Review #45

Taha-Hassan-Git opened this issue Oct 26, 2023 · 0 comments

Comments

@Taha-Hassan-Git
Copy link

Love the project, nice idea and very well-executed.

  • The code is super easy to read, all the components are named very clearly and the folder structure makes sense. You've even got an underscore before your _lib folder, nice.q

  • In a page.js file I'm generally expecting it to be a server component that does some data fetching and returns a stack of semantically-named components. This helps a developer that's new to the project to be able to tell at a glance what the page is and what it does. You've done that really nicely here:

export default function Page() {
  const listings = getAll()
  return (
    <>
      {listings.map((listing) => {
        return <Listing key={listing.id} {...listing} />
      })}
    </>
  )
}

...but it's less clear on other pages. Not a big deal at all, and definitely something that could fall by the wayside when you're working at the pace you guys have to. I'd reccomend turning more of the HTML into components to enhance readability.

  • Very impressed that you got some tests out, well done to the QA for finding time to do that.

  • I also like the random image on the homepage, great feature

  • Looks like you're using some legacy props for the Image component and that's causing some console errors. You're also doing some data-fetching in a funky place on the homepage. I might do something like this instead:

export default function Home() {

// Move the data-fetching up here
//fetch the descriptions as well for our alt text
  const {image, description} = getRandomImage()

  return (
   // some components
          <Image
//use css to set the cover
// Also position the image top left so we aren't zoomed in on squidward's crotch
// though that might just be a personal preference
            className='object-cover object-left-top'
            priority
            src={image}
            fill={true}
            // this is to tell Next what the size of the container is
            sizes='60vw'
            alt={`${description}`}
          />
   // more components
 }

Something you'll see a lot in larger codebases that you don't necessarily need to worry about for now is something like this:

import Image from 'next/image'
import Link from 'next/link'
import { getRandomImage } from './_lib/models/random-image.js'

export default function Home() {
//if the server is down for whatever reason, we might get an undefined here, 
///we don't want the whole site to crash though.
  const image = getRandomImage()
  return (
// this renders the image if it's there, but renders a fallback if not
          {image ? <Image
            className='object-cover object-left-top'
            priority
            src={image.image}
            fill={true}
            sizes='60vw'
            alt={`${image.description}`}
          /> : <FallbackComponent/>}
}
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