Style selector basic functionality #76
Conversation
Optional chaining is used to avoid errors resulting from lookups on undefined.
slargman
added
the
implementation task
Optional chaining is used to avoid errors resulting from lookups on undefined.
|
|
||
| return ( | ||
| <div> | ||
| { stylesArray } |
There was a problem hiding this comment.
Put this directly in here for easier readability. But seems this component is maybe not even needed as it's essentially a map function over the <Style> component. Is this <StyleSelector> going into a lot of places?
There was a problem hiding this comment.
No, StyleSelector is only used once. I did it like this to try to compartmentalize things and keep Overview simple by just including StyleSelector. Would you recommend replacing <StyleSelector /> by the map in Overview?
There was a problem hiding this comment.
Actually, I realize now that's not even what I'm doing. Taking your other comment into account, I'll move <StyleSelector /> to <Overview />.
| {/*TODO: show Product Overview*/} | ||
| {/*TODO: add Share buttons*/} | ||
| {selectedStyle?.original_price} | ||
| {selectedStyle?.sale_price !== 0 ? selectedStyle?.sale_price : null} |
There was a problem hiding this comment.
Seems this is specific to one product, so maybe <StyleSelector> shouldn't be inside of it. Would help with not having to pass down so many props to so many descendants
|
|
||
| setProduct(productResponse.data); | ||
| setStyles(sortedStyles); | ||
| setSelectedStyleId(sortedStyles.find((style) => style['default?']).style_id); | ||
| setSelectedStyleId( |
There was a problem hiding this comment.
You can initialize state lazily: https://reactjs.org/docs/hooks-reference.html#lazy-initial-state
There was a problem hiding this comment.
Does this provide an advantage over useEffect without subscribers? I also need to set product and styles from data that I pull from an API. But the data is fetched only on the initial render since the subscribers array is empty. I'm now realizing that the useEffect hook should run when productId changes though, since the data should be refetched whenever a new product is being displayed.
Sorry, should've explained this better. Overall this is what my
There are issues that describe the specific functionality that the components and their subcomponents should have. I'll link the |
|
Actually I don't want to close those issues yet, since they're still works in progress, but I'll link the relevant issues in the top message of the PR. |
|
|
||
| function StyleSelector({ styles, selectedStyleId, handleStyleSelect }) { | ||
| // TODO: use color-thief to extract average color of thumbnails server-side | ||
| const stylesArray = styles.map((style) => ( |
There was a problem hiding this comment.
You can just directly return this and skip setting to a var then calling it later
There was a problem hiding this comment.
Yes good point. I think I had separated it in case I had more complicated logic in the return statement, but that ended up not being the case.
Also makes changes for accessibility.
Red:
StyleSelectorProductInformationStyleSelector<-- (this PR is for this component)AddToCartImageGalleryProductDetail(this may go inProductInformationultimately)Relevant to: