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

Task Catalog #4199

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

Task Catalog #4199

wants to merge 7 commits into from

Conversation

HigorPedroso
Copy link

Copy link

@pedro-ruas pedro-ruas left a comment

Choose a reason for hiding this comment

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

Higor, your code looks good, but there are small things to change.
One issue that I should point is the fact that when hovering one card, all the next line gets pushed down. Checklist asks it not to happen, can you also think a way to solve it?

>
Laptops & Computers
</a>
</li>

Choose a reason for hiding this comment

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

Please remember to add empty lines between sibling elements, it is requested in Code Styles Rules

src/index.html Outdated
</h2>
<p class="card__code">Product code: 195434</p>
<div class="card__rate">
<div class="card__rate-stars stars">

Choose a reason for hiding this comment

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

You could have declared this .stars block in a similar way as you have done in Stars task.
In the way that is implemented there, there is a modifier on stars which shows the adequate amount of active and inactive stars, as opposed to what you've made here where the styling is fixed

Choose a reason for hiding this comment

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

The empty missing empty line between siblings repeats several times here, be sure to check the whole file when fixing it, ok?


&:hover {
transform: scale(1.2);
transition-duration: 300ms;

Choose a reason for hiding this comment

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

To keep the transition smooth when hovering an element, the transition property should be declared not inside the :hover state, but in the element itself.
You should move this declaration to .card to fix this problem.
Also, you can use the transition property instead of transition-duration

height: 134px;
box-sizing: border-box;
margin-left: 3px;
transition: 3s;

Choose a reason for hiding this comment

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

Is this transition being used?

&:hover &__title {
color: #34568b;
cursor: pointer;
transition: 3s;

Choose a reason for hiding this comment

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

The styling here is right, but a 3s transition for a hover, are you sure? Maybe you should check it in the Checklist

color: #fff;
margin-top: 16px;
cursor: pointer;
text-transform: uppercase;

Choose a reason for hiding this comment

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

And here you are missing a transition property, so your transition when hover doesn't look abrupt

Copy link

Choose a reason for hiding this comment

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

Still unfixed, please make sure all your hover effects are smooth Reference

width: 16px;
margin-right: 4px;

&:nth-child(5) {

Choose a reason for hiding this comment

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

I've mentioned this before, but try changing this approach so you don't have fixed styles that forces all products to be 4 stars active

@HigorPedroso HigorPedroso requested a review from pedro-ruas March 11, 2024 22:50
Copy link

@EdPirro EdPirro left a comment

Choose a reason for hiding this comment

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

Great effort, @HigorPedroso! There is just one fix pointed out by @pedro-ruas that you may have overlooked and your solution should be good to go! 🚀

Comment on lines +54 to +59
&-reviews {
font-size: 10px;
font-weight: 400;
line-height: 14px;
}
}
Copy link

Choose a reason for hiding this comment

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

Avoid defining one element inside another, although __rate and __rate-reviews share the beginning of the name they're actually two different elements, this nesting may cause confusion;

Same happens in __price, __price-text and __price-value
This is a personal suggestion and not a required change

color: #fff;
margin-top: 16px;
cursor: pointer;
text-transform: uppercase;
Copy link

Choose a reason for hiding this comment

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

Still unfixed, please make sure all your hover effects are smooth Reference

@HigorPedroso HigorPedroso requested a review from EdPirro March 12, 2024 17:01
readme.md Outdated Show resolved Hide resolved
Copy link

@EdPirro EdPirro left a comment

Choose a reason for hiding this comment

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

Awesome job, @HigorPedroso! Keep it up 🚀

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