Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

made catalog #4850

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

made catalog #4850

wants to merge 2 commits into from

Conversation

fendered
Copy link

  • [x ] All components follow BEM and use SCSS
  • [x ] Repeated sizes and special colors are put to variables
  • [x ] Grid is used for the columns
  • [x ] Cards are shown in 1, 2, 3, or 4 columns based on screen resolution
  • x[ ] All changes on :hover are smooth
  • [x ] Code follows all the Code Style Rules ❗️

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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


It looks like there are a few critical issues that need to be addressed before we can approve your pull request. The repeated use of href="#" is a significant concern as it affects the navigation functionality. This needs to be replaced with valid URLs or handled with JavaScript to ensure proper navigation. Additionally, please ensure that all necessary images and SCSS modules are present in the specified directories to avoid missing styles or assets. Once these issues are resolved, your code will be in great shape! Keep up the good work and don't hesitate to reach out if you need any help. 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

src/index.html Show resolved Hide resolved
src/index.html Show resolved Hide resolved
src/index.html Show resolved Hide resolved
src/index.html Show resolved Hide resolved
src/index.html Show resolved Hide resolved
src/index.html Show resolved Hide resolved
src/index.html Show resolved Hide resolved
src/index.html Show resolved Hide resolved
src/styles/index.scss Show resolved Hide resolved
src/styles/modules/card.scss Show resolved Hide resolved
Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Sorry, the pull request review was unsuccessful for the following reasons:

Warning

No changes were made in the pull request since the last review. Please make sure that you have added files to the pull request and committed the changes.
[CODE: 7]

If the issue is within your control, please address it.
The next review attempt will automatically launch in 1 hour.
If you believe this is an error or need assistance, please contact the support team.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

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

Thank you for your submission! 🎉 Your code has been reviewed and is approved to help you progress in your learning journey. However, there are a few important issues that need attention:

  1. Navigation Links: The use of href="#" in navigation links can lead to unexpected behavior. Please replace these with valid URLs or handle them with JavaScript to ensure proper navigation.

  2. HTML Structure: There's a misplaced href attribute within a li element. Make sure it is correctly placed inside an a tag to function properly as a link.

  3. SCSS and BEM: Ensure that all components consistently use BEM naming conventions as required by the task.

  4. Data Attributes: Verify that the data-qa attributes are correctly applied, especially the data-qa="nav-hover" on the 4th nav link.

  5. Grid Layout: Confirm that the grid layout is responsive and adapts to different screen resolutions as intended.

  6. Resource Verification: Double-check that all paths to imported modules, font files, and images are correct and that these resources exist in the specified directories.

These improvements are crucial for the functionality and accessibility of your project. Keep up the great work, and use this feedback to enhance your skills! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

<li
class="header__link"
data-qa="nav-hover"
href="/laptop-Computers"

Choose a reason for hiding this comment

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

The href attribute is incorrectly placed within the li element. It should be inside the a tag to function properly as a link.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

<p class="right">$2,199</p>
</div>
<a
href="#"

Choose a reason for hiding this comment

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

Using href="#" is not recommended as it can cause unexpected behavior. Consider replacing it with a valid URL or handling the click event with JavaScript.

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.

2 participants