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

product card 24.12.2024 #5114

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

product card 24.12.2024 #5114

wants to merge 4 commits into from

Conversation

YriKm
Copy link

@YriKm YriKm commented Dec 24, 2024

No description provided.

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.


Great job on your implementation! 🎉 Your code meets the task requirements and is well-structured. Just make sure that the Parcel bundler is correctly set up to handle SCSS files, as this will ensure everything compiles smoothly without runtime issues. Keep up the excellent work and happy coding! 😊

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

Comment on lines 23 to 26
<link
rel="stylesheet"
href="./styles/index.scss"
/>

Choose a reason for hiding this comment

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

Ensure that the Parcel bundler is set up to handle SCSS files correctly, as you are linking to an SCSS file directly. This is allowed, but make sure the setup is correct to avoid runtime issues.

Copy link

@volodymyr-soltys97 volodymyr-soltys97 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 👍
Let's improve your code
You need to fix language on the card, check it on the design
image

readme.md Show resolved Hide resolved
src/index.html Outdated
Comment on lines 52 to 58
<div class="star__line star__line--0">
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
</div>

Choose a reason for hiding this comment

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

Fix classes and implement the modifier logic

Suggested change
<div class="star__line star__line--0">
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
</div>
<div class="stars stars--4">
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
<div class="stars__star"></div>
</div>

class="card"
data-qa="card"
>
<div class="card__cord-image">

Choose a reason for hiding this comment

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

Do you need this wrapper for image?

Suggested change
<div class="card__cord-image">

Copy link
Author

Choose a reason for hiding this comment

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

Without it I couldn't center the image.

src/index.html Outdated
Comment on lines 60 to 88
<div class="star__line star__line--1">
<div class="star__style star__style--active"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
</div>

<div class="star__line star__line--2">
<div class="star__style star__style--active"></div>
<div class="star__style star__style--active"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
</div>

<div class="star__line star__line--3">
<div class="star__style star__style--active"></div>
<div class="star__style star__style--active"></div>
<div class="star__style star__style--active"></div>
<div class="star__style star__style--passive"></div>
<div class="star__style star__style--passive"></div>
</div>

<div class="star__line star__line--4">
<div class="star__style star__style--active"></div>
<div class="star__style star__style--active"></div>
<div class="star__style star__style--active"></div>
<div class="star__style star__style--active"></div>

Choose a reason for hiding this comment

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

Remove other blocks

src/index.html Outdated
</div>

<button
class="card__buy-button button"

Choose a reason for hiding this comment

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

Suggested change
class="card__buy-button button"
class="card__button"

src/index.html Outdated Show resolved Hide resolved
Copy link

@VitaliyBondarenko1982 VitaliyBondarenko1982 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.
Do not forget add demo links to PR description in next tasks.

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