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.

Sign up for GitHub

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

Jump to bottom

catalog hover version 1 #2015

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

catalog hover version 1 #2015

wants to merge 6 commits into from

Conversation

steveforde
Copy link

Anastasiia-Svintsova
Anastasiia-Svintsova suggested changes Jul 30, 2023
View reviewed changes
Copy link

@Anastasiia-Svintsova Anastasiia-Svintsova left a comment

Choose a reason for hiding this comment

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

  • Title color currently does not change on hover
    image

src/styles/blocks/catalog.scss Outdated
width: 200px;
height: 300px;
border: 1px solid #ccc;
border-radius: 5px;
margin-bottom: 48px;
transition: all $hoverTransitionDuration;
Copy link

Choose a reason for hiding this comment

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

Specify the transition property. Using all as a transition property is not the best approach

Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/styles/blocks/header.scss Outdated
Comment on lines 1 to 2
$hoverTransitionDuration: 300ms;
$linkTextColorOnHover: #00acdc;
Copy link

Choose a reason for hiding this comment

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

Create a separate file for variables

src/styles/blocks/catalog.scss Outdated
Comment on lines 29 to 32
transform: scale($cardScaleOnHover);
.card-title {
color: $titleTextColorOnHover;
}
Copy link

Choose a reason for hiding this comment

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

Suggested change
transform: scale($cardScaleOnHover);
.card-title {
color: $titleTextColorOnHover;
}
transform: scale($cardScaleOnHover);
color: $titleTextColorOnHover;

Copy link

Choose a reason for hiding this comment

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

Not fixed
image

Anastasiia-Svintsova
Anastasiia-Svintsova suggested changes Jul 31, 2023
View reviewed changes
Copy link

@Anastasiia-Svintsova Anastasiia-Svintsova left a comment

Choose a reason for hiding this comment

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

Please, make sure you fixed all the comments

src/index.html Outdated
@@ -60,31 +60,31 @@

<main class="catalog">
<div class="catalog__card">
Product card
<p class="card-title">Product card</p>
Copy link

Choose a reason for hiding this comment

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

You don't need to change the html file. Just use the correct styles. Discard all changes in this file, please

src/styles/blocks/catalog.scss Outdated
Comment on lines 29 to 32
transform: scale($cardScaleOnHover);
.card-title {
color: $titleTextColorOnHover;
}
Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/styles/blocks/header.scss Outdated
@@ -32,11 +32,15 @@
}
}

&__link {
.nav__link {
Copy link

Choose a reason for hiding this comment

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

No need to change the selector here

Suggested change
.nav__link {
.&__link {

Copy link

Choose a reason for hiding this comment

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

Not fixed
image

Copy link

Choose a reason for hiding this comment

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

Pls fix this)

src/styles/blocks/header.scss Outdated
Comment on lines 41 to 42
transition: color $hoverTransitionDuration;
&:hover {
Copy link

Choose a reason for hiding this comment

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

Suggested change
transition: color $hoverTransitionDuration;
&:hover {
transition: color $hoverTransitionDuration;
&:hover {

Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/styles/blocks/variables.scss
@@ -0,0 +1,4 @@
$hoverTransitionDuration: 300ms;
$linkTextColorOnHover: #00acdc;
$cardScaleOnHover: 1.2;
Copy link

Choose a reason for hiding this comment

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

You don't really need to create a variable for the scale value, you use it only once. But it's not a mistake, you can leave it like that

Anastasiia-Svintsova
Anastasiia-Svintsova suggested changes Jul 31, 2023
View reviewed changes
Copy link

@Anastasiia-Svintsova Anastasiia-Svintsova left a comment

Choose a reason for hiding this comment

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

Please, make sure you fixed all previous comments. Feel free to ask questions in the chat

src/styles/blocks/catalog.scss Outdated
width: 200px;
height: 300px;
border: 1px solid #ccc;
border-radius: 5px;
margin-bottom: 48px;
transition: all $hoverTransitionDuration;
Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/styles/blocks/header.scss Outdated
@@ -32,11 +32,15 @@
}
}

&__link {
.nav__link {
Copy link

Choose a reason for hiding this comment

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

Not fixed
image

src/styles/blocks/header.scss Outdated
Comment on lines 41 to 42
transition: color $hoverTransitionDuration;
&:hover {
Copy link

Choose a reason for hiding this comment

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

Not fixed
image

lerastarynets
lerastarynets suggested changes Jul 31, 2023
View reviewed changes
Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

Almost there) Just several more things needs to be fixed

src/styles/blocks/header.scss Outdated

.nav {
&__list {
.nav__list {
Copy link

Choose a reason for hiding this comment

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

pls make use of sass nesting for nav selector like so
.nav {
&__list {
...
}
&__item {
...
}
}

src/styles/blocks/header.scss Outdated
@@ -32,11 +32,15 @@
}
}

&__link {
.nav__link {
Copy link

Choose a reason for hiding this comment

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

Pls fix this)

@steveforde steveforde requested a review from lerastarynets July 31, 2023 14:04
lerastarynets
lerastarynets suggested changes Jul 31, 2023
View reviewed changes
Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

pls pay attention to .nav__link and the comments to it
and try to make use of nesting

@steveforde steveforde requested a review from lerastarynets July 31, 2023 14:58
lerastarynets
lerastarynets approved these changes Jul 31, 2023
View reviewed changes
Copy link

@lerastarynets lerastarynets left a comment

Choose a reason for hiding this comment

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

You've got this, keep pushing forward!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lerastarynets lerastarynets lerastarynets approved these changes

@Anastasiia-Svintsova Anastasiia-Svintsova Awaiting requested review from Anastasiia-Svintsova

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@steveforde @lerastarynets @Anastasiia-Svintsova