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

What a cute idea! ❤️ #26

Open
fairyaksh opened this issue May 21, 2021 · 0 comments
Open

What a cute idea! ❤️ #26

fairyaksh opened this issue May 21, 2021 · 0 comments

Comments

@fairyaksh
Copy link

fairyaksh commented May 21, 2021

I'm gonna leave a list of possible issues (you might already be aware of some, in which case ignore me) below:

  • This one's easy to miss but don't forget to remove comments in files (index.js, products.js, [id].js, Layout.js)
  • There's an empty curly bracket in your component's params in checkout.js - don't forget to remove if it's not being used 😉
  • Get rid of empty/unused files - example.env .prettierrc (or don't if you are still planning to set this up..)
  • Great use of file/function modularisation here - glad to see my last week comments were noted!!
  • All your images not being served on prod site - have you npm run build or whatever your build script is called so it can serve the updated ver to your prod site?
  • Cypress - keep one file as an example if you need but if you're not testing at all or haven't been able to, feel free to delete the files
  • Nice readme! Good to see effort put into that :) One extra thing I'd add is in the local installation section - mention the step to npm install (it might be second nature for some devs but still could have someone locally installing your proj confused why nothing works!)
  • Rollernuts icon - remove all min & max widths/heights on the image and just keep/manipulate 'width' to decrease icon size (right now it's big enough that it displaces the title 'rollernuts' and makes your nav looks misaligned)
  • /products path gives a server error - no database url env var - check you have dotenv file correctly imported, etc. Might require some debugging to see why the error is being thrown.
  • Kinda unrelated to your code but I tried to go to localhost:3000/api/hello - but I get a 404 not found? Would be good to check if this issue is being reproduced on your computers or if it's just my local setup having problems?
  • Your team has done some excellent work! And there has been a loooot to take in the past two weeks, but it would nice to see a more responsive website too, although it's understandable that it isn't a priority atm!
  • When I click on 'cart' on your nav, the console shows these two errors:
    1. validateDOMNesting(...): <div> cannot appear as a descendant of <p>. A <p></p> tag can only contain inline elements. That means putting a <div></div> tag inside it should be improper, since the div tag is a block element. Improper nesting might cause glitches like rendering extra tags, which can affect your javascript and css. Try using <span> instead of <div> inside your <p>. OR just use <p> inside divs rather than the other way around that your code has now. (credit: this stackoverflow answer)
    2. react-dom.development.js?61bb:67 Warning: Expected server HTML to contain a matching <div> in <a>. - according to this error, there seems to be a closing tag missing somewhere.

I'm really liking the concept of this project and the cleanness of your file structure. Well done for working so hard this week, NextJS is no easy feat so be proud of yourselves! I'm really curious to see what this project may look like fully fleshed out in the future...! 😼 💓

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant