-
Notifications
You must be signed in to change notification settings - Fork 0
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
2. As a user, I want to be able to navigate within the app #42
Conversation
I see that at some point you renamed the hello.js file, but looks like maybe that rename didn't fully make it into git? Did you miss a As I was typing this I just remembered that git is not especially great at renaming files if all you are changing is the casing (e.g. from One solution here may be to change the file name entirely, maybe give that a shot? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some nice work @mikeramz86 @prophen—great first PR 👏👏👏
I got the feature running locally after changing the casing of src/hello.js
.
Very nice to see the commits broken up as you did. @prophen that last commit message was ✨ 🚀 ❤️
src/App.js
Outdated
</div> | ||
); | ||
} | ||
|
||
export default App; | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see your thought process @mikeramz86! I think comments like this are better in the ticket or the PR description. In general, it's fine to have a the occasional TODO comment like // TODO: Redesign Hello Component
But I wouldn't leave a bigger TODO List in comments like this.
src/AppRouter.js
Outdated
import { Route, Switch } from 'react-router-dom'; | ||
import Hello from './Hello'; | ||
import HelloAgain from './HelloAgain'; | ||
// import App from './App'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can delete this line 🔥
src/AppRouter.js
Outdated
@@ -1,6 +1,6 @@ | |||
import React from 'react'; | |||
import { Route, Switch } from 'react-router-dom'; | |||
import Hello from './hello'; | |||
import Hello from './Hello'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch with the name capitalization. Like @larsbrekken mentioned, we'll need to change the name of the file as well now from src/hello.js
to src/Hello.js
src/index.js
Outdated
@@ -1,12 +1,24 @@ | |||
// import React from 'react'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can all get removed 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is some nice work @mikeramz86 @prophen—great first PR 👏👏👏
I got the feature running locally after changing the casing of src/hello.js
.
Very nice to see the commits broken up as you did. @prophen that last commit message was ✨ 🚀 ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My comment posted twice
src/HelloAgain.js
Outdated
|
||
function HelloAgain() { | ||
return ( | ||
<div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a blocker by any means, but for better accessibility, you might consider using semantic HTML here as a div
essentially has no meaning. I might suggest using main
here in this case. You also could use a React Fragment if you ever come across a case where you need to wrap elements for some reason but they don't really fit with any of the semantic tags.
…able to render new page
- Removed the boilerplate React homepage - Created a separate component, for each page we link to. Hello.js and HelloAgain.js - Renamed the hello.js to Hello.js I think the issue was that we were trying to route to the same component that the Router was wrapped around. I don't know if my implementation is the most correct solution, but a way around that problem was to give our home page '/' it's own component (Hello.js) to render the new view separate from App.js.
This commit merges in the firebase configuration that was recently merged with master. This will allow us to toggle between a list view and an add-item view.
d951103
to
f5b174f
Compare
✅ react-router-dom has been added as a project dependency
✅ Click a link within the app to show a different view in the app
✅ When a link is clicked, the browser URL updates to represent the current view
Questions:
switch
orreact-router-dom
?#24