Skip to content
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

Style selector basic functionality #76

Merged
merged 22 commits into from May 25, 2022
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 14 additions & 9 deletions client/src/components/Overview/Overview.jsx
Expand Up @@ -3,6 +3,8 @@ import PropTypes from 'prop-types';
import axios from 'axios';
import ImageGallery from 'Overview/ImageGallery.jsx';
import ProductInformation from 'Overview/ProductInformation.jsx';
import StyleSelector from 'Overview/StyleSelector.jsx';
import AddToCart from 'Overview/AddToCart.jsx';
import testData from 'tests/testData.js';

function Overview({ productId }) {
Expand All @@ -19,13 +21,15 @@ function Overview({ productId }) {
axios.get(`/products/${productId}/styles`),
]);

const sortedStyles = stylesResponse.data.results.sort((style1, style2) => (
style1.styled_id - style2.styled_id
));
const sortedStyles = stylesResponse.data.results.sort(
(style1, style2) => style1.styled_id - style2.styled_id
);

setProduct(productResponse.data);
setStyles(sortedStyles);
setSelectedStyleId(sortedStyles.find((style) => style['default?']).style_id);
setSelectedStyleId(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@slargman slargman May 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sortedStyles.find((style) => style['default?']).style_id
);
} catch (error) {
// TODO: handle error
}
Expand All @@ -40,18 +44,19 @@ function Overview({ productId }) {

return (
<div>
<ImageGallery
selectedStyleId={selectedStyleId}
/>
<ImageGallery selectedStyleId={selectedStyleId} />
<ProductInformation
// product={product}
product={testData.product}
selectedStyle={styles.find((style) => style.style_id === selectedStyleId)}
/>
<StyleSelector
// styles={styles}
selectedStyleId={selectedStyleId}
handleStyleSelect={handleStyleSelect}
// TODO: delete once fetching is working
product={testData.product}
styles={testData.styles.results}
/>
<AddToCart />
</div>
);
}
Expand Down
21 changes: 4 additions & 17 deletions client/src/components/Overview/ProductInformation.jsx
@@ -1,15 +1,10 @@
import React from 'react';
import PropTypes from 'prop-types';
import StyleSelector from 'Overview/StyleSelector.jsx';
import AddToCart from 'Overview/AddToCart.jsx';

/**
* Shows general product information
*/
function ProductInformation({ product, styles, selectedStyleId, handleStyleSelect }) {
// TODO: how to avoid optional chaining?
const selectedStyle = styles?.find((style) => style.style_id === selectedStyleId);

function ProductInformation({ product, selectedStyle}) {
return (
<div>
{/*TODO: add rating stars*/}
Expand All @@ -22,25 +17,17 @@ function ProductInformation({ product, styles, selectedStyleId, handleStyleSelec
Style &gt; {selectedStyle?.name}
<br />
{/*TODO: add strikethrough for sale*/}
{selectedStyle?.original_price}
{selectedStyle?.sale_price !== 0 ? selectedStyle?.sale_price : null}
{/*TODO: show Product Overview*/}
{/*TODO: add Share buttons*/}
<StyleSelector
styles={styles}
selectedStyleId={selectedStyleId}
handleStyleSelect={handleStyleSelect}
/>
<AddToCart />
{selectedStyle?.original_price}
{selectedStyle?.sale_price !== 0 ? selectedStyle?.sale_price : null}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

</div>
);
}

ProductInformation.propTypes = {
product: PropTypes.object.isRequired,
styles: PropTypes.array.isRequired,
selectedStyleId: PropTypes.number.isRequired,
handleStyleSelect: PropTypes.func.isRequired,
selectedStyle: PropTypes.object.isRequired,
};

export default ProductInformation;
10 changes: 4 additions & 6 deletions client/src/components/Overview/ProductInformation.test.jsx
Expand Up @@ -3,21 +3,19 @@ import ProductInformation from 'Overview/ProductInformation.jsx';

describe('ProductInformation', () => {
it('should show the price for the selected style', () => {
const styles = testData.styles.results;

const { rerender } = render(<ProductInformation
product={testData.product}
styles={testData.styles.results}
selectedStyleId={1}
handleStyleSelect={() => {}}
selectedStyle={styles.find((style) => style.style_id === 1)}
/>);

let node = screen.getByText(/(?<=Style > )(.+?)$/);
expect(node).toHaveTextContent('Forest Green & Black');

rerender(<ProductInformation
product={testData.product}
styles={testData.styles.results}
selectedStyleId={2}
handleStyleSelect={() => {}}
selectedStyle={styles.find((style) => style.style_id === 2)}
/>);

node = screen.getByText(/(?<=Style > )(.+?)$/);
Expand Down
48 changes: 46 additions & 2 deletions client/src/components/Overview/StyleSelector.jsx
@@ -1,7 +1,51 @@
import React from 'react';
import PropTypes from 'prop-types';

function StyleSelector() {
return null;
/**
* Displays thumbnails of the product styles and
* allows style selection by clicking on a thumbnail
*/
function Style({ style, handleStyleSelect, selected }) {
// TODO: handle selected overlay
return (
<div onClick={() => handleStyleSelect(style.style_id)}>
<img
src={style.photos[0].thumbnail_url}
alt={`${style.name} style thumbnail`}
width="50"
/>
</div>
);
}

Style.propTypes = {
style: PropTypes.object.isRequired,
handleStyleSelect: PropTypes.func.isRequired,
selected: PropTypes.bool.isRequired,
};

function StyleSelector({ styles, selectedStyleId, handleStyleSelect }) {
// TODO: use color-thief to extract average color of thumbnails server-side
const stylesArray = styles.map((style) => (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just directly return this and skip setting to a var then calling it later

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

<Style
key={style.style_id}
style={style}
handleStyleSelect={handleStyleSelect}
selected={style.styled_id === selectedStyleId}
/>
));

return (
<div>
{ stylesArray }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I realize now that's not even what I'm doing. Taking your other comment into account, I'll move <StyleSelector /> to <Overview />.

</div>
);
}

StyleSelector.propTypes = {
styles: PropTypes.array.isRequired,
selectedStyleId: PropTypes.number.isRequired,
handleStyleSelect: PropTypes.func.isRequired,
};

export default StyleSelector;
33 changes: 33 additions & 0 deletions client/src/components/Overview/StyleSelector.test.jsx
@@ -0,0 +1,33 @@
import { screen } from '@testing-library/react';
import StyleSelector from 'Overview/StyleSelector.jsx';

describe('StyleSelector', () => {
it('should change selected style when a thumbnail is clicked', async () => {
const user = userEvent.setup();
const handleStyleSelect = (styleId) => {
selectedStyleId = styleId;
};
let selectedStyleId = 1;

const {rerender} = render(<StyleSelector
styles={testData.styles.results}
selectedStyleId={selectedStyleId}
handleStyleSelect={handleStyleSelect}
/>);

//TODO: might need to be more specific as thumbnails get added to imageGallery
const style = 'Desert Brown & Tan';
await user.click(screen.getByAltText(new RegExp(style)));

const selectedStyleObj = Object.values(testData.styles.results)
.find(styleObj => styleObj.style_id === selectedStyleId);
const selectedStyle = selectedStyleObj.name;

expect(selectedStyle).toBe(style);
});

it.todo('should show all styles for a product');
it.todo('should initially select the default style <- should go in Overview')
it.todo('should not change style when the currently selected style thumbnail is clicked');
it.todo('should indicate the selected style with an overlay');
});