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 #35

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

Taha's code review #35

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

Comments

@Taha-Hassan-Git
Copy link

Very cool project, love the idea and also very impressed at how far you got with it.

  • Grest state management! Would be nice to see the variant tracked in state as well, this would help you eventually calculate the real total. Though you may have to switch to an object rather than array for that. Something like this:
const [basket, setBasket] = useState([{politician: "1", variant: "a"}, {politician: "2", variant: "b"}])
  • Project was super easy to figure out. Great component naming, variable naming, folder structure etc. I like that you've kept all you lib/db stuff out of the app folder 👍

  • The styling is really nice too, generally great use of contrast.

  • We've squashed our politicians! Try this:

          <Image
              className="object-cover"
              src={servant.profile_img}
// could we put a more meaningful alt here?
              alt={`A profile picture of ${servant.name}`}
              fill={true}
              priority
            />

...also consider making RenderServants and RenderFiltered the same component, or have a new component called something like PoliticianCard. This means you don't have to make changes to both files when you fix something like this.

  • We could do more object destructuring throughout the codebase, just to make it a bit easier to read, not a big deal though:
export default function RenderIndividualServant({ params }) {
  const id = params.id
  const {name, profile_img, base_price, about} = selectServantByID(id)
  const variants = listVariants()

  return (
        <h3>{name}</h3>
        <Image
          src={profile_img}
          alt="Servant Profile Picture"
          width={200}
          height={200}
          priority
        />
        <p>Price: £{base_price}</p>
      <p>{about}</p>
      <p>What would you like {name} to do for you? </p>
      <VariantSelect variants={variants} base_price={base_price} />
      <AddToCart servant_name={name} />
    </>
  )
}
  • Great work with this, I'm super impressed!
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