Conversation
| backgroundColor: theme.palette.disabledCard.background, | ||
| color: theme.palette.disabledCard.text, | ||
| color: defaultTextColor, | ||
| padding: `${theme.spacing(1 / 2)} ${theme.spacing(1)}`, |
There was a problem hiding this comment.
there isnt a way to add color from pallete ? (following mui documentation)
|
|
||
| import { Divider, Grid } from '@mui/material'; | ||
| import { Grid, Box, Divider } from '@mui/material'; |
There was a problem hiding this comment.
no use of Box and Divider, please remove
There was a problem hiding this comment.
Pull Request Overview
This PR redesigns the cart shopping interface with a new layout and styling approach. The changes transform the cart from a vertical layout to a modern two-column design with improved visual hierarchy and typography variants.
Key changes include:
- Introduction of new text variants for better typography consistency across cart components
- Complete restructuring of cart layout from vertical to horizontal grid-based design
- Simplified product card component by removing unused props and improving status handling
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/constants/types.js | Adds new text variant constants for cart typography and removes trailing whitespace |
| src/constants/hardText.js | Adds new cart-related text constants and status values |
| src/components/shared/Text/textVariants.js | Defines styling for new text variants with specific typography rules |
| src/components/Product/ProductCard.jsx | Simplifies component by removing unused props and improving status comparison |
| src/components/Product/Product.styles.js | Updates styling to use consistent text colors and removes unused theme parameter |
| src/components/Cart/Cart.styles.js | Complete redesign of cart styling with new grid layout and card-based design |
| src/components/Cart/Cart.jsx | Restructures cart component to use new two-column layout with improved product display |
| export const ProductImage = { | ||
| width: '150px', | ||
| height: '150px', | ||
| objectFit: 'cover', | ||
| borderRadius: '14px', | ||
| marginRight: '2rem', | ||
| }; |
There was a problem hiding this comment.
ProductImage should be a styled component function like other exports in this file, rather than a plain object. This creates inconsistency in the styling approach.
| export const ProductImage = { | |
| width: '150px', | |
| height: '150px', | |
| objectFit: 'cover', | |
| borderRadius: '14px', | |
| marginRight: '2rem', | |
| }; | |
| export const ProductImage = styled('img')(() => ({ | |
| width: '150px', | |
| height: '150px', | |
| objectFit: 'cover', | |
| borderRadius: '14px', | |
| marginRight: '2rem', | |
| })); |
| export const ProductName = styled('div')(() => ({ | ||
| fontWeight: 800, | ||
| fontSize: '1.6rem', | ||
| marginBottom: '1rem', | ||
| textTransform: 'capitalize', | ||
| })); |
There was a problem hiding this comment.
ProductName styled component duplicates styling that already exists in textVariants.js (PRODUCT_TITLE). Consider using the existing text variant instead of creating duplicate styles.
| export const ProductName = styled('div')(() => ({ | |
| fontWeight: 800, | |
| fontSize: '1.6rem', | |
| marginBottom: '1rem', | |
| textTransform: 'capitalize', | |
| })); | |
| // Use <SharedTypography variant="PRODUCT_TITLE" /> for product names instead of a custom styled component. |
| export const ProductPrice = styled('div')(() => ({ | ||
| fontWeight: 700, | ||
| fontSize: '1.4rem', | ||
| color: '#111', | ||
| marginBottom: '1rem', | ||
| })); |
There was a problem hiding this comment.
ProductPrice styled component duplicates styling that already exists in textVariants.js (PRODUCT_PRICE). Consider using the existing text variant instead of creating duplicate styles.
| export const ProductPrice = styled('div')(() => ({ | |
| fontWeight: 700, | |
| fontSize: '1.4rem', | |
| color: '#111', | |
| marginBottom: '1rem', | |
| })); | |
| export const ProductPrice = styled(SharedTypography)({ | |
| marginBottom: '1rem', | |
| }); |
| export const ProductLocation = styled('div')(() => ({ | ||
| fontSize: '1.1rem', | ||
| color: 'gray', | ||
| marginBottom: '2rem', | ||
| })); |
There was a problem hiding this comment.
ProductLocation styled component duplicates styling that already exists in textVariants.js (PRODUCT_LOCATION). Consider using the existing text variant instead of creating duplicate styles.
| export const ProductLocation = styled('div')(() => ({ | |
| fontSize: '1.1rem', | |
| color: 'gray', | |
| marginBottom: '2rem', | |
| })); | |
| export const ProductLocation = styled(SharedTypography).attrs({ | |
| variant: 'PRODUCT_LOCATION', | |
| })({ | |
| marginBottom: '2rem', | |
| }); |
| export const SummaryTitle = styled('div')(() => ({ | ||
| fontWeight: 800, | ||
| fontSize: '1.5rem', | ||
| marginBottom: '2rem', | ||
| })); |
There was a problem hiding this comment.
SummaryTitle styled component duplicates styling that already exists in textVariants.js (SECTION_TITLE). Consider using the existing text variant instead of creating duplicate styles.
| export const SummaryTitle = styled('div')(() => ({ | |
| fontWeight: 800, | |
| fontSize: '1.5rem', | |
| marginBottom: '2rem', | |
| })); | |
| // Use SharedTypography with variant="SECTION_TITLE" instead of SummaryTitle |
| export const SummaryRow = styled('div')(() => ({ | ||
| display: 'flex', | ||
| justifyContent: 'space-between', | ||
| fontSize: '1.2rem', | ||
| marginBottom: '1.2rem', | ||
| })); |
There was a problem hiding this comment.
SummaryRow styled component duplicates styling that already exists in textVariants.js (SUMMARY_ROW). Consider using the existing text variant instead of creating duplicate styles.
| export const SummaryRow = styled('div')(() => ({ | |
| display: 'flex', | |
| justifyContent: 'space-between', | |
| fontSize: '1.2rem', | |
| marginBottom: '1.2rem', | |
| })); | |
| // Removed duplicate SummaryRow styled component. Use SharedTypography with variant="SUMMARY_ROW" instead. |
| export const SummaryTotal = styled('div')(() => ({ | ||
| display: 'flex', | ||
| justifyContent: 'space-between', | ||
| fontWeight: 800, | ||
| fontSize: '1.4rem', | ||
| })); |
There was a problem hiding this comment.
SummaryTotal styled component duplicates styling that already exists in textVariants.js (SECTION_TOTAL). Consider using the existing text variant instead of creating duplicate styles.
| export const SummaryTotal = styled('div')(() => ({ | |
| display: 'flex', | |
| justifyContent: 'space-between', | |
| fontWeight: 800, | |
| fontSize: '1.4rem', | |
| })); | |
| export const SummaryTotal = styled(SharedTypography).attrs({ | |
| variant: 'SECTION_TOTAL', | |
| })({ | |
| display: 'flex', | |
| justifyContent: 'space-between', | |
| }); |
No description provided.