ethanheyrman / restaurant-roulette

0 stars 1 forks source link

Home page setup #22

Closed jkuzminski7 closed 4 years ago

jkuzminski7 commented 4 years ago

Created style for Home page matching random page style and base for home page layout

BijanT commented 4 years ago

I have a few comments:

1) It seems like the standard practice when cloning an npm repo is to run npm install which reads package.json to install the dependencies. When I do that then run npm start, I get the following

./src/App.js Module not found: Can't resolve 'react-router-dom' in '/home/bijan/Documents/Homework/CS506/restaurant-roulette/frontend/src'

I know that this was likely a problem before the PR, but could you add react-router-dom to the package.json dependencies list? Either in this PR or a separate one is fine

2) What's the difference between App.js and Homepage.js? It seems like they are trying to be the same thing. I also can't find a way to navigate to the content on Homepage.js, because the URL localhost:3000/ just sends me to the default page of App.js

I would recommend removing Homepage.js and moving what it added to App.js.

I haven't looked too deeply into the code, so my suggestions could be off base. Just let me know

NiharikaTomar commented 4 years ago

@BijanT I am getting an npm error too when I try to rebase my branch off of Josh's. However, it is a slightly different error than yours. I get an error saying that there is ‘no package.json found and so it can’t audit a project’ when I try to run the npm start command. Do you think this is something that is happening because of the issue you pointed out? or is it because of something completely different?

BijanT commented 4 years ago

@NiharikaTomar What directory are you running npm start from? It should be run in the frontend directory which should have a package.json file

NiharikaTomar commented 4 years ago

@NiharikaTomar What directory are you running npm start from? It should be run in the frontend directory which should have a package.json file

oh yeah, running it from the frontend dir and then it gives me that error :( just thought it's maybe something related to the issue you mentioned

BijanT commented 4 years ago

If you want to fix the error, you can run npm install react-router-dom from the frontend directory. My suggestion in my original comment is just to make bringing up a newly cloned version of the repo easier.

NiharikaTomar commented 4 years ago

If you want to fix the error, you can run npm install react-router-dom from the frontend directory. My suggestion in my original comment is just to make bringing up a newly cloned version of the repo easier.

ok for some weird reason installing the router-dom fixed that package.json error and the thing starts up! Thanks, @BijanT :) I just think we should change the page's color scheme because the random page on home screen gets camouflaged with the background once you click on that link but we can probs get to that later

NiharikaTomar commented 4 years ago

@jkuzminski7 looks good for merging. We need @BenHolzem to give a go-ahead if he is ok with this frontend stuff for @ethanheyrman to merge.

BenHolzem commented 4 years ago

@jkuzminski7 looks good to me @ethanheyrman if you want to merge it

ethanheyrman commented 4 years ago

merging, say goodbye