Week 3: Homework by koss-lebedev · Pull Request #5 · strvcom/react-nights-2019

koss-lebedev · 2019-04-03T15:01:24Z

Hi guys,

while doing this homework, I realized that there are a few things that I should probably include as part of the work that I did during the lecture, because some of the changes aren't really related to the homework itself. For instance, I want to prepare the following things so that students wouldn't have to worry about them:

update API for getProduct to return a product in the same shape as we get them in getProducts API (don't know why it's different now)
add Layout to ProductDetail page
extract generic Button component

The aforementioned changes are in this PR right now, but I think that I should make them myself so that students could build on top of them. What do you think?

varholak-peter · 2019-04-03T16:01:57Z

+      <Layout>
+        <Wrapper>
+          {this.state.isLoading && <Loader />}
+          {this.props.product && (


Since we are passing the product through the props now, it makes no sense to keep a single boolean value in the state for loading. Instead, it makes more sense to do something like:

{!this.props.product ? <Loader /> : <>The Actual product rendering</>}

As with the current implementation, we could potentially run into a condition where we display both the product detail and the loader.

varholak-peter · 2019-04-03T16:03:30Z

+      <Layout>
+        <Wrapper>
+          {this.state.isLoading && <Loader />}
+          {this.props.product && (


There is a lot of this.props.product how about adding more destructuring for improved readability?

varholak-peter · 2019-04-03T16:07:44Z

  }

  async componentDidMount() {
-    if (this.props.products.length === 0) {


Do we want to remove this if?
I presume that since it was shown during the presentation a lot of students will keep it in their code and it will pass the HW review (unless we want to force mentors to make sure they throw it out) and suddenly we have different functionality for other presenters when presenting.
If we want to remove this part I would recommend doing that during the lesson or announcing it in the Slack channel, so we don't run into sneaky bugs due to this.

We do, because otherwise if we load product detail page first, and then navigate to product list, we'll have one product to the store and the full list won't be loaded.

Good point, we should mention that in Slack then so students aren't surprised by this.

We can mention that we showed this as a sample of caching thanks to global state, yet for a more production ready setup you would need a more complex implementation.

* Add create-customer API call * Implement sign up form with styles and libraries * General layout improvement * ESLint and stylelint configuration fixes

dannytce

Overall great job! :) But I found a couple of things I would like to address too. Btw @varholak-peter had great point too!

Some of the issues are written as comments per line. Some of them I am going address here:

in ProductDetail we changed to logic how to display <Loader />. Let's use the same logic for ProductList
Let's rename store/cartItems into store/cart
Let's use in store reducer.js instead of index.js
It's not related to this PR, but we can probably address it here. There are exactly same data transformation in get-products.js and get-product.js. So first of all, can we have just products.js or products/index.js having both functionalites there? Secondly, can we abstract those data transforms into helper function? :)

Otherwise I really like extra stuff you did. You are correct it's not related to the homework, but could be beneficial for our students. Let's communicate that in Slack! :)

dannytce · 2019-04-08T12:13:48Z

 })

-const Cart = connect(mapStateToProps)(CartView)
+const actionCreators = {


It's actually mapDispatchToProps. Naming doesn't matter here at all, but for students, it might be better for studying purposes... Because it actually does the mapping of dispatch to props :)

Also, the official documentation is using the same term :)

https://react-redux.js.org/next/api/connect#connect-parameters

I think the documentation says that actionCreators is also an acceptable name here, since we don't do the actual "mapping" as we would with a more verbose syntax.

Since the actual name of the variable is up to you, you might want to give it a name like actionCreators, or even define the object inline in the call to connect

https://react-redux.js.org/using-react-redux/connect-mapdispatch#defining-mapdispatchtoprops-as-an-object

Haha, fair point! :P But still, but still I am more into mapDispatchToProps :D

dannytce · 2019-04-08T12:21:15Z

-      <AddButton onClick={evt => onAddToCart(node.id, evt)}>
-        Add to Cart
-      </AddButton>
+      <Button onClick={evt => onAddToCart(node.id, evt)}>Add to Cart</Button>


Actually, let's make it more future proofed. The correct behavior supposed to be that onAddToCart shouldn't deal with anything else than the actual adding product to cart. Thus, let's have it as following:

<Button onClick={event => { event.preventDefault(); onAddToCart(node.id) }}>Add to Cart</Button>

This is a bit better because parent component doesn't have to care about the implementation.

Please try to avoid using shortcuts for common parameters, such as event. It's straightforward for us, but again confusing for students.

dannytce · 2019-04-08T12:29:16Z

-const store = createStore(reducer)
+const store = createStore(
+  reducer,
+  window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__()


Let's have a comment there explaining where we are getting those devtools from :)

I think it's totally fine just to have there a link to: https://github.com/zalmoxisus/redux-devtools-extension#11-basic-store

dannytce · 2019-04-08T12:30:31Z

@@ -1,6 +1,12 @@
 export const LOAD_PRODUCTS = 'products/LOAD'
+export const LOAD_PRODUCT = 'products/LOAD_ONE'


This is not good for debugging. Let's be more explicit by using the same names of constants

export const LOAD_PRODUCTS = 'products/LOAD_PRODUCTS' export const LOAD_PRODUCT = 'products/LOAD_PRODUCT'

Might be a little late, but could we change the naming LOAD_PRODUCT/S to SET_PRODUCT/S? I was confused when reading my student code that he was calling the API and then calling loadProducts, this word LOAD usually implies side effects so is more related to fetching in my opinion than using SET

dannytce · 2019-04-08T12:44:24Z

    case LOAD_PRODUCTS:
      return action.payload
+    case LOAD_PRODUCT: {
+      const restProducts = state.filter(p => p.id !== action.payload.id)


Maybe let's write it more clean and easier to follow?

return state.map(product => product.id === action.payload.id ? action.payload : product)

Also, an explanation left in a comment, why we did this would be awesome. I assume we are doing that, because of the following:

There might be already some products loaded (e.g. from product list)

get product detail returns more data than product list

The mapping logic won't work if the original state is empty. But I'll add an explanatory comment, that's a great idea!

Zaggen · 2019-04-08T14:43:28Z

        ...state,
        [action.payload]: (state[action.payload] || 0) + 1,
      }
+    case REMOVE_PRODUCT: {


Shouldn't this first decrease the quantity before getting removed? Maybe you just want to remove the duplicate that you accidentally added

@Zaggen you are absolutely right! But let's keep this functionality to the next lessons.

prichodko

Just left one comment, but no strong preferences.

Edit: It won't work as well, your solution is bulletproof. 👍 😄

Week 3: Homework

c85213e

koss-lebedev requested review from prichodko and varholak-peter April 3, 2019 15:01

varholak-peter reviewed Apr 3, 2019

View reviewed changes

apedroferreira added a commit that referenced this pull request Apr 7, 2019

Week 4: Forms (#5)

c0bcaf3

* Add create-customer API call * Implement sign up form with styles and libraries * General layout improvement * ESLint and stylelint configuration fixes

Clean up ProductDetail component

206023b

dannytce suggested changes Apr 8, 2019

View reviewed changes

Additional refactoring

39b2b5c

Zaggen reviewed Apr 8, 2019

View reviewed changes

dannytce approved these changes Apr 8, 2019

View reviewed changes

prichodko approved these changes Apr 8, 2019

View reviewed changes

Comment thread src/store/products/reducer.js

dannytce merged commit 0bd872d into master Apr 9, 2019

dannytce deleted the week3-homework branch April 9, 2019 08:34

		@@ -1,6 +1,12 @@
		export const LOAD_PRODUCTS = 'products/LOAD'
		export const LOAD_PRODUCT = 'products/LOAD_ONE'

Conversation

koss-lebedev commented Apr 3, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dannytce left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zaggen Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prichodko left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Zaggen Apr 8, 2019 •

edited

Loading

prichodko left a comment •

edited

Loading