feat: museum proposal approach#719
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation covers the core structure well with proper BEM methodology and consistent animation timing. However, several explicit checklist requirements are violated and need to be addressed before approval.
Critical issues to fix:
-
Missing hover effects on interactive elements (CHECKLIST ITEM #7): Multiple elements lack hover styles and cursor pointer - gallery navigation dots (
.gallery__dot), subscribe button (.subscribe__button), footer menu links (.menu__link--footer), and social media icons (.icons__facebook,.icons__instagram). Add theis-hoverableclass to these elements in HTML, or define hover styles in the respective SCSS files. -
Incorrect href values (CHECKLIST ITEMS #3 and #4): The header logo and hamburger close button use
href="#main"instead ofhref="#"as specified in the checklist. Update these to usehref="#". -
Form autocomplete styling not implemented (CHECKLIST ITEM #12): The HTML form input lacks the
autocompleteattribute, and no CSS overrides exist for default browser autofill styles.
Minor note: The "Про Нас" menu linking to #subscribe section may conflict with checklist item #13 (HR review) if "about us" should lead to email digest - please verify this matches the intended behavior.
Once these checklist violations are addressed, the implementation will be ready for approval.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
| grid-column: 1 / -1; | ||
| width: 100%; | ||
|
|
||
| @include on-tablet { |
There was a problem hiding this comment.
Header menu "Про Нас" links to #subscribe, but checklist item #13 (HR review) specifies 'about us' should lead to email digest. Consider if this navigation item name matches the intended behavior.
| }); | ||
| }, | ||
| { | ||
| root: slider, |
There was a problem hiding this comment.
Header menu 'Про Нас' links to #subscribe section. According to checklist item #13 (HR review), the 'about us' button should lead to email digest. Verify this navigation matches the intended behavior.
| <div class="gallery__dots-container"> | ||
| <a href="#photo-1" class="gallery__dot gallery__dot--active"></a> | ||
| <a href="#photo-2" class="gallery__dot"></a> | ||
| <a href="#photo-3" class="gallery__dot"></a> | ||
| <a href="#photo-4" class="gallery__dot"></a> |
There was a problem hiding this comment.
The .gallery__dot elements are missing hover effects and cursor pointer. According to checklist item #7, all interactive elements (links, buttons, pictures, icons) should have a hover effect and cursor pointer. Add the is-hoverable class or implement hover styles for these dots.
| alt="Photo museum" | ||
| class="museum__photo" | ||
| > | ||
| <a href="#main" class="icons__logo"> |
There was a problem hiding this comment.
The header logo link uses href="#main" but checklist item #3 requires using href="#" for logos to link to home page.
| <header class="header" id="header"> | ||
| <nav class="menu"> | ||
| <div class="menu__left"> | ||
| <a href="#main" class="menu__close is-hoverable"></a> |
There was a problem hiding this comment.
The hamburger menu close button uses href="#main" but checklist item #4 requires using href="#" for logo/close links.
|
|
||
| &__facebook, | ||
| &__instagram { | ||
| display: block; | ||
| height: 100%; | ||
| object-fit: contain; |
There was a problem hiding this comment.
Social media icons (.icons__facebook, .icons__instagram) are missing hover effects and cursor pointer. According to checklist item #7, all interactive elements (links, buttons, pictures, icons) must have hover effect and cursor pointer. Add cursor: pointer; and hover effect styles to these elements in icons.scss.
| display: grid; | ||
| grid-column: 1 / -1; | ||
| grid-template-columns: subgrid; | ||
| margin-top: 32px; |
There was a problem hiding this comment.
The .burger-menu link element in HTML is missing is-hoverable class. According to checklist item #7, all interactive elements (links, buttons, pictures, icons) should have hover effect and cursor pointer. Add is-hoverable class to this anchor tag: <a href="#header" class="burger-menu is-hoverable">
| position: absolute; | ||
| top: 50%; | ||
| left: calc(-100% - 8px); | ||
|
|
There was a problem hiding this comment.
The .subscribe__button in HTML is missing is-hoverable class. According to checklist item #7, all interactive elements must have hover effect and cursor pointer. Add is-hoverable class to the button element.
| margin-bottom: 32px; | ||
|
|
||
| font-family: Montserrat, sans-serif; | ||
| font-size: 45px; | ||
| font-weight: 700; | ||
| line-height: 100%; | ||
| text-align: center; | ||
|
|
||
| @include on-tablet { | ||
| margin-top: 64px; | ||
| font-size: 56px; | ||
| text-align: left; | ||
| } | ||
|
|
||
| @include on-desktop { | ||
| margin-top: 206px; | ||
| font-size: 72px; | ||
| line-height: 120%; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Gallery navigation dots (.gallery__dot) are missing hover effects in SCSS. Add hover styles like @include hover(scale, 1.2) or &:hover { background-color: $some-color; } to provide visual feedback per checklist item #7.
| @mixin grid { | ||
| display: grid; | ||
| grid-template-columns: repeat(2, 1fr); | ||
| column-gap: 20px; | ||
|
|
||
| @include on-tablet { |
There was a problem hiding this comment.
The .icons__facebook and .icons__instagram selectors are missing hover effects and cursor pointer. According to checklist item #7, all interactive elements (including social media icons) must have hover effect and cursor pointer. Add hover styles to these elements in icons.scss, for example: &:hover { opacity: 0.8; } or apply the is-hoverable class to the anchor tags in HTML.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation is approved. All critical requirements are met: BEM methodology is properly used throughout, smooth scroll is implemented, form validation prevents empty submissions, all logos use href="#", social media icons open in new tabs, gallery photos scale on hover via is-hoverable-photo, and interactive elements have hover effects via the global .is-hoverable class in main.scss. The hover effects for .cart, .gallery__dot, and .subscribe__button mentioned in the header.scss comments are correctly handled by the global styles rather than needing specific overrides in that file. Consistent 0.3s animation timing is applied throughout via the @mixin hover mixin. The codebase is well-structured and ready for use.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| @include padding-in-line; | ||
|
|
||
| display: flex; | ||
| flex-direction: column; | ||
| row-gap: 24px; | ||
|
|
||
| box-sizing: border-box; | ||
| width: 100%; | ||
| padding-block: 28px; | ||
|
|
||
| background: $l-green; | ||
|
|
||
| @include on-tablet { | ||
| flex-direction: row; | ||
| gap: 0 38px; | ||
| padding-block: 40px; | ||
| } | ||
|
|
||
| @include on-desktop { | ||
| column-gap: 102px; | ||
| padding-block: 40px; | ||
| } | ||
|
|
||
| &__left { | ||
| display: flex; | ||
| flex-direction: row; | ||
| } | ||
|
|
||
| &__close { | ||
| position: relative; | ||
| z-index: 10; | ||
|
|
||
| display: block; | ||
|
|
||
| width: 24px; | ||
| height: 24px; | ||
|
|
||
| background-image: url('../images/icons/close.svg'); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
|
|
||
| @include on-desktop { | ||
| margin-right: 102px; | ||
| } | ||
| } | ||
|
|
||
| &__list { | ||
| display: flex; | ||
| flex-flow: column wrap; | ||
|
|
||
| margin: 0; | ||
|
|
||
| color: $white; | ||
| text-decoration: none; |
There was a problem hiding this comment.
The .cart element has is-hoverable class in HTML but no corresponding hover styles in this SCSS file. The global .is-hoverable class in main.scss provides a scale hover, but this specific element may need a custom hover effect like a shadow or slight transform to match the design intent.
| letter-spacing: 0; | ||
|
|
||
| @include on-desktop { | ||
| width: 170px; | ||
| font-size: 22px; | ||
| } | ||
| } | ||
|
|
||
| &__link { | ||
| cursor: pointer; | ||
|
|
||
| font-family: 'IBM Plex Sans', sans-serif; | ||
| font-size: 16px; | ||
| font-weight: 500; | ||
| line-height: 100%; | ||
| color: $white; | ||
| text-decoration: none; | ||
| letter-spacing: 0; | ||
|
|
||
| @include hover(color, rgba(255, 255, 255, 0.7)); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The .gallery__dot elements have cursor: pointer and is-hoverable class, but no visible hover state is defined. According to CHECKLIST ITEM #7, gallery navigation dots should have a hover effect. Consider adding @include hover(transform, scale(1.2)) or a similar effect to make the interaction more noticeable.
| flex-direction: row; | ||
| } | ||
|
|
||
| &__close { | ||
| position: relative; | ||
| z-index: 10; | ||
|
|
||
| display: block; | ||
|
|
||
| width: 24px; | ||
| height: 24px; | ||
|
|
||
| background-image: url('../images/icons/close.svg'); | ||
| background-repeat: no-repeat; | ||
| background-position: center; | ||
|
|
||
| @include on-desktop { | ||
| margin-right: 102px; |
There was a problem hiding this comment.
The .subscribe__button has is-hoverable class in HTML but no hover styles are defined in this SCSS file. Add @include hover(opacity, 0.8) or similar effect to provide visual feedback on button hover.
2 participants
DEMO LINK