-
Notifications
You must be signed in to change notification settings - Fork 0
Cookbook #6
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
base: master
Are you sure you want to change the base?
Cookbook #6
Conversation
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.
Here several functionalities are not implemented:
- Sign-in, you have to send the user to the server and validate if such user is in DB
- You have to make validation on routes navigation and redirect the user if user not authorized
- You can implement adding the recipe, you can just store them offline
- Same about the ingredients. It looks like you app not interactive at all
cookbook/cookbook/src/App.js
Outdated
import {HeaderComponent} from "./shared-components/header-component"; | ||
|
||
class App extends Component { | ||
// constructor() { |
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.
please remove commented code
} | ||
}; | ||
|
||
changeInputUserName = (event) => { |
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.
it could be improved this way: https://reactjs.org/docs/forms.html#handling-multiple-inputs
We did something like this in class
1c14904
to
77361f2
Compare
import {clearUser} from "../store/actions/user-action/clear-user"; | ||
|
||
const AuthUserWrapper = (WarappedComponent) => { | ||
class Base extends Component { |
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.
pretty good, it could be improved this way
componentDidMount() {
if(!this.props.user) {
thi.props.history.push('/autorization-page')
}
}
clearUser | ||
}; | ||
|
||
export default connect(mapStateToProps, mapDispatchToProps)(withRouter(AutorizationComponent)); |
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.
looks like you router could not work properly due to documentation:
https://reacttraining.com/react-router/web/guides/redux-integration
it could be withRouter(connect()())
It's better to have named exports instead of export default connect. In your scenario it could be hard to debug in react-dev-tool
return recept.id === nextProps.match.params.id | ||
}); | ||
if(coincidentalRecept.length === 0) { | ||
nextProps.history.push('/recepts'); |
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.props.history is fine to use. Function would have the same link
return elem.id !== action.payload.recept.id | ||
}); | ||
return newState; | ||
} | ||
case TRIGGER_INGRIDIENT_STATE: { | ||
return state.map(recept => { | ||
if(recept.id === action.payload.receptId) { |
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.
it's better to move such logic to action and simplify a reducer to just or so
return action.payload.state
}); | ||
} | ||
case REMOVE_INGREDIENT: { | ||
return state.map(recept => { |
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.
same as above
}); | ||
} | ||
case UPDATE_LIST_RECEPTS: { | ||
return state.map(recept => { |
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.
same as above
|
||
return | ||
case INGREDIENTS_TRIGGER: { | ||
let newIngredients = state.map(ingredient => { |
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.
please move logic to action instead of reducer
return( | ||
<ol> | ||
{ | ||
recept.ingredients && recept.ingredients.map((ingredient, index) => { |
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.
it worth to move it to method something like renderReducer
You should omit to have a logic inside JSX
return done; | ||
}, []); | ||
|
||
if(receptName.length === 0) { |
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.
Pretty sure it could be simplified, something like this
if(receptName.length) {
return receptName
}
if(ingridientName.length) {
return ingridientName
}
return []
Seems like it's the same
No description provided.