Skip to content

catalog grid 1#1924

Open
steveforde wants to merge 8 commits into
mate-academy:masterfrom
steveforde:develop
Open

catalog grid 1#1924
steveforde wants to merge 8 commits into
mate-academy:masterfrom
steveforde:develop

Conversation

@steveforde
Copy link
Copy Markdown

Copy link
Copy Markdown

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

Good job,
try to make that you card dont stuck to the side (try to use justify-content: center; in catalog)
image

@steveforde steveforde requested a review from maxim2310 August 3, 2023 08:34
Copy link
Copy Markdown

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Cards should be centred horizontally on your page for all the media queries, however now it's not(
Pls fix

Copy link
Copy Markdown

@Anastasiia-Svintsova Anastasiia-Svintsova left a comment

Choose a reason for hiding this comment

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

  • It seems that media queries don't work correctly now. There should be three columns when the screen width is 1023px and two at 767px:
    image
    image

  • Align the cards container to the center
    image

  • Gap between cards should be 48px.
    image

Copy link
Copy Markdown

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

you cards still aren't centered, remove this rules for fix
image

@steveforde steveforde requested a review from maxim2310 August 3, 2023 16:44
Copy link
Copy Markdown

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

@steveforde steveforde requested a review from maxim2310 August 3, 2023 17:10
Copy link
Copy Markdown

@maxim2310 maxim2310 left a comment

Choose a reason for hiding this comment

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

Fix comment and must to work

Comment thread src/styles/_catalog.scss
grid-gap: 48px;
max-width: 944px;
margin: 0 auto;
justify-content: center;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
justify-content: center;
justify-items: center;

@steveforde steveforde requested a review from maxim2310 August 4, 2023 10:38
Copy link
Copy Markdown

@Anastasiia-Svintsova Anastasiia-Svintsova left a comment

Choose a reason for hiding this comment

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

  • There are still redundant spaces between the cards
    image

Comment thread src/styles/_catalog.scss Outdated
align-items: center;

@media (min-width: 1024px) {
grid-template-columns: repeat(4, 1fr);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Here is the reason. Create a variable for the card width and use it
$card-width: 200px;

Suggested change
grid-template-columns: repeat(4, 1fr);
grid-template-columns: repeat(4, $card-width);

Comment thread src/styles/_catalog.scss Outdated
Comment on lines +36 to +39
.catalog__card:nth-child(3n + 1),
.catalog__card:nth-child(3n + 2),
.catalog__card:nth-child(3n + 3) {
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This code seems redundant

Copy link
Copy Markdown

@witflash witflash left a comment

Choose a reason for hiding this comment

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

Well done 👍

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.

5 participants