Hooks version #18
Hooks version #18
Conversation
| this.setState({ | ||
| error: false, | ||
| }); | ||
| const withErrorHandler = (WarppedComponent, axios) => props => { |
There was a problem hiding this comment.
I like the dependency injection of axios
I'm curious what inspired you to pass it in like that.
There was a problem hiding this comment.
It is just way listing to Axios call before reach then, catch
AKA global error handler
| mounted.current = true; | ||
| return () => { | ||
| axios.interceptors.request.eject(axiosRequest); | ||
| axios.interceptors.response.eject(axiosResponse); |
There was a problem hiding this comment.
Damn this is some advanced axios API moves -- neat!
|
For some reason, GitHub action -- the build failed to Webpack console warns @leggomuhgreggo keep that in mind |
mohqarmout
added
Done
question
|
Hope this PR satisfy you I did my best to mack it optimal |
There was a problem hiding this comment.
Solid work!
I felt bad or taking so long to review so I was pretty thorough, but hope it doesn't give the wrong impression -- the changes are dank 👍
There might be one or two spots where you could eliminate a hook if the code organized a little different but still totally works.
LMK if you have any questions 👍
| 'prettier/react', | ||
| 'plugin:jsx-a11y/recommended', | ||
| 'plugin:react-redux/recommended', | ||
| 'plugin:jest/recommended', | ||
| 'plugin:jest/style' | ||
| 'plugin:jest/style', | ||
| 'plugin:prettier/recommended', |
There was a problem hiding this comment.
Note (no change required): One thing about the prettier plugin is that -- if some other plugin has formatting rules that conflict with prettier -- whichever plugin is later in the extends: [...] array will override the other. So keeping prettier near the last is a good practice 👍
| <main className={classes.Content}>{children}</main> | ||
| </> | ||
| ); | ||
| }; |
| valid: false, | ||
| touched: false, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
suggestion: I'd recommend extracting initial state -- keeping config separate is going to make life easier
| authError, | ||
| authenticated, | ||
| authRedirect, | ||
| buildingBurger, |
There was a problem hiding this comment.
suggestion (non-blocking): This is slightly out of scope but -- it's good practice to observe the boolean naming convention where you prefix boolean vars with simple verbs like "is" "has" "can" etc.
Incidentally, your naming choices are really solid -- like way better than most of the code I see 👍
| if (buildingBurger) { | ||
| authSetRedirectPath('/checkout'); | ||
| } else { | ||
| authSetRedirectPath('/'); | ||
| } | ||
| } | ||
| }, [buildingBurger]); |
|
|
||
| updatePurchaseState = () => { | ||
| const { ingredients } = this.props; | ||
| const updatePurchaseState = useCallback(() => { |
| const disabledInfo = { | ||
| }, [ingredients]); | ||
|
|
||
| let orderSummary = null; |
There was a problem hiding this comment.
question: Is this state?
| }, [ingredients]); | ||
|
|
||
| let orderSummary = null; | ||
| let burger = <Spinner />; |
There was a problem hiding this comment.
suggestion: might try to rewrite this as a ternary either inline or an extracted render method -- that way you can skip the mutation.
| orderSummary = ( | ||
| <OrderSummary | ||
| ingredients={ingredients} | ||
| }, [ingredients]); |
There was a problem hiding this comment.
suggestion: Interesting! Hmm might be a slightly indirect control flow mechanism but totally works
| for (const key in disabledInfo) { | ||
| if ({}.hasOwnProperty.call(disabledInfo, key)) { | ||
| disabledInfo[key] = disabledInfo[key] <= 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
nitpick: I'd try to avoid for loops if possible since it's usually a sing of mutation and the syntax is a little gnarly. Also curious what inspired the hasOwnProperty approach?
mohqarmout
added
fast-follow
and removed
Awaiting Review
Trying to figure out Hooks & apply it to the core project