-
Notifications
You must be signed in to change notification settings - Fork 263
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
Frontend/react codebase #20
Frontend/react codebase #20
Conversation
Thanks for opening this pull request! |
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.
@jainpawan21 Can you please deploy into Netlify and share the link in PR description?
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.
@jainpawan21 Added a few comments, please have a look.
body { | ||
margin: 0; | ||
background-color: #fafafa; | ||
font-family: 'Montserrat', sans-serif; |
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.
@jainpawan21 This font won't load.
Import this font. Check this.
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.
These fonts are loading perfectly and working fine!
Should I download local fonts?
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.
No, can't we import?
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.
I forgot to copy that link. Fixed Now :)
import {FETCH_TANKS} from './types'; | ||
|
||
export const fetchTanks = () => { | ||
console.log('in actions') |
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.
@jainpawan21 Please remove the logs.
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.
Removed!
react-frontend/src/App.test.js
Outdated
|
||
test('renders learn react link', () => { | ||
const { getByText } = render(<App />); | ||
const linkElement = getByText(/learn react/i); |
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.
@jainpawan21 This test will fail as <App />
doesn't have learn react
text anymore. Please update this.
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.
Removed App.test.js!
export default function(state = initialState, action) { | ||
switch (action.type) { | ||
case FETCH_TANKS: | ||
console.log('In reducer') |
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.
@jainpawan21 Please remove this log.
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.
Removed!
Congrats on merging your first pull request! 🙌🎉⚡️ |
This PR converts main web page into ReactJs. I have used react-redux for state management.
Initial conversion of website into reactt
Please delete options that are not relevant.
Checklist:
Reviewer: Vinit Shahdeo