Skip to content

Conversation

@OrtalNisim
Copy link
Contributor

No description provided.

Comment on lines 36 to 38
backgroundColor: theme.palette.disabledCard.background,
color: theme.palette.disabledCard.text,
color: defaultTextColor,
padding: `${theme.spacing(1 / 2)} ${theme.spacing(1)}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

there isnt a way to add color from pallete ? (following mui documentation)

Comment on lines -2 to +1
@@ -4,115 +1,132 @@
import { Grid, Box, Divider } from '@mui/material';
Copy link
Contributor

Choose a reason for hiding this comment

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

no use of Box and Divider, please remove

@OfekSagiv OfekSagiv requested a review from Copilot August 6, 2025 15:36
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +56 to +62
export const ProductImage = {
width: '150px',
height: '150px',
objectFit: 'cover',
borderRadius: '14px',
marginRight: '2rem',
};
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

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.

Suggested change
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',
}));

Copilot uses AI. Check for mistakes.
Comment on lines +68 to 73
export const ProductName = styled('div')(() => ({
fontWeight: 800,
fontSize: '1.6rem',
marginBottom: '1rem',
textTransform: 'capitalize',
}));
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

ProductName styled component duplicates styling that already exists in textVariants.js (PRODUCT_TITLE). Consider using the existing text variant instead of creating duplicate styles.

Suggested change
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.

Copilot uses AI. Check for mistakes.
Comment on lines +75 to +80
export const ProductPrice = styled('div')(() => ({
fontWeight: 700,
fontSize: '1.4rem',
color: '#111',
marginBottom: '1rem',
}));
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

ProductPrice styled component duplicates styling that already exists in textVariants.js (PRODUCT_PRICE). Consider using the existing text variant instead of creating duplicate styles.

Suggested change
export const ProductPrice = styled('div')(() => ({
fontWeight: 700,
fontSize: '1.4rem',
color: '#111',
marginBottom: '1rem',
}));
export const ProductPrice = styled(SharedTypography)({
marginBottom: '1rem',
});

Copilot uses AI. Check for mistakes.
Comment on lines +82 to +86
export const ProductLocation = styled('div')(() => ({
fontSize: '1.1rem',
color: 'gray',
marginBottom: '2rem',
}));
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

ProductLocation styled component duplicates styling that already exists in textVariants.js (PRODUCT_LOCATION). Consider using the existing text variant instead of creating duplicate styles.

Suggested change
export const ProductLocation = styled('div')(() => ({
fontSize: '1.1rem',
color: 'gray',
marginBottom: '2rem',
}));
export const ProductLocation = styled(SharedTypography).attrs({
variant: 'PRODUCT_LOCATION',
})({
marginBottom: '2rem',
});

Copilot uses AI. Check for mistakes.
Comment on lines +98 to +102
export const SummaryTitle = styled('div')(() => ({
fontWeight: 800,
fontSize: '1.5rem',
marginBottom: '2rem',
}));
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

SummaryTitle styled component duplicates styling that already exists in textVariants.js (SECTION_TITLE). Consider using the existing text variant instead of creating duplicate styles.

Suggested change
export const SummaryTitle = styled('div')(() => ({
fontWeight: 800,
fontSize: '1.5rem',
marginBottom: '2rem',
}));
// Use SharedTypography with variant="SECTION_TITLE" instead of SummaryTitle

Copilot uses AI. Check for mistakes.
Comment on lines +104 to +109
export const SummaryRow = styled('div')(() => ({
display: 'flex',
justifyContent: 'space-between',
fontSize: '1.2rem',
marginBottom: '1.2rem',
}));
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

SummaryRow styled component duplicates styling that already exists in textVariants.js (SUMMARY_ROW). Consider using the existing text variant instead of creating duplicate styles.

Suggested change
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.

Copilot uses AI. Check for mistakes.
Comment on lines +111 to +116
export const SummaryTotal = styled('div')(() => ({
display: 'flex',
justifyContent: 'space-between',
fontWeight: 800,
fontSize: '1.4rem',
}));
Copy link

Copilot AI Aug 6, 2025

Choose a reason for hiding this comment

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

SummaryTotal styled component duplicates styling that already exists in textVariants.js (SECTION_TOTAL). Consider using the existing text variant instead of creating duplicate styles.

Suggested change
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',
});

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants