Skip to content

add task solution#762

Open
Iryna8989 wants to merge 2 commits into
mate-academy:masterfrom
Iryna8989:develop
Open

add task solution#762
Iryna8989 wants to merge 2 commits into
mate-academy:masterfrom
Iryna8989:develop

Conversation

@Iryna8989
Copy link
Copy Markdown

@Iryna8989 Iryna8989 commented May 22, 2026

Copy link
Copy Markdown

@brespect brespect left a comment

Choose a reason for hiding this comment

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

Good progress, but you need to add your DEMO LINK tot he PR description

@Iryna8989
Copy link
Copy Markdown
Author

Done, I’ve added the demo link to the PR description

@Iryna8989 Iryna8989 requested a review from brespect May 22, 2026 09:07
Copy link
Copy Markdown

@brespect brespect left a comment

Choose a reason for hiding this comment

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

Good progress, almost done:

  1. Menu should have no scrolls:
Image
  1. Add underline hover to this buttons:
Image

@Iryna8989
Copy link
Copy Markdown
Author

Thank you, I have a couple of questions regarding the review comments.

  1. "Menu should have no scrolls"
    I'm not sure what exactly is meant here. I don't see any scrollbars in the menu on my side, and I also don't see a visible scrollbar in the screenshot from the review.
    I already disable page scrolling when the menu is open:

&:has(.page__menu:target) {
overflow: hidden;
}

If the comment refers to:

.menu {
overflow: auto;
}

I added it intentionally as a fallback in case the menu contains more items in the future and doesn't fit into the viewport. As far as I remember, we used the same approach in a previous project. Could you please clarify what should be changed here?

  1. I tried two implementations and I'm not sure which one is expected:
  • the underline appears when hovering the entire product card (since the whole card is clickable).
  • the underline appears only when hovering the "Order" button itself.

At the moment I left the second option. Which approach would you prefer for this project?

@Iryna8989 Iryna8989 requested a review from brespect May 22, 2026 17:13
Copy link
Copy Markdown

@etojeDenys etojeDenys 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

i think mentor considered this solution which you used in the previous task

Image

@Iryna8989
Copy link
Copy Markdown
Author

Thank you for the clarification.

Just to note, this solution was already implemented before the review comment was left:

.page:has(.page__menu:target) {
overflow: hidden;
}

This code has been in the project from the beginning and page scrolling is already disabled when the menu is open.

Could you please clarify if there is another issue with the menu that should be fixed?

@Iryna8989 Iryna8989 requested a review from etojeDenys May 22, 2026 21:55
Copy link
Copy Markdown

@vitalii-bondarenko-ua vitalii-bondarenko-ua left a comment

Choose a reason for hiding this comment

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

Looks good

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.

4 participants