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

Develop #356

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

Develop #356

wants to merge 8 commits into from

Conversation

OstinM
Copy link

@OstinM OstinM commented Oct 25, 2023

Copy link

@roman-mirzoian roman-mirzoian 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, but there is quite a bit of work to be done.

According to the task, you must have certain mandatory sections, which are not all present at the moment (it is enough to change the naming):
image

The element for changing the language should appear in one line:
image

The "Send" button should not take us anywhere at the moment:
https://gyazo.com/fdc2aaf3f162dc922e28eeac190160bf

@include onDeskTop {
@include hover(transform, scale(105%));
}

Choose a reason for hiding this comment

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

Suggested change

grid-column: span 12;
}

// column-gap: 30px;

Choose a reason for hiding this comment

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

Do not leave unnecessary comments.

&__info-list {
padding-inline: 30px;
margin: 0 0 50px 14px;
@include onTablet {

Choose a reason for hiding this comment

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

Suggested change
@include onTablet {
@include onTablet {

grid-column: 1/ 5;
margin: 0 0 0 14px;
padding: 0;

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 142 to 143


Choose a reason for hiding this comment

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

Suggested change

Comment on lines 1 to 15
$c-gray: #eee;
$c-menu-item: #f9f9f9;
$c-white: #fff;
$main-text-c: #000;
$second-text-c: #7c7c7c;
$btn-c: #333;
$btn-hover: #131313;
$page-background-c: #f7f7f7;
$see-more-link-c:#0db2b3;
$contarast-c: #d6ecec;
$dark-green-c: #0c797a;
$form-border-c:#828282;
$placeholder-c: #bdbdbd;
$disabled-btn-c: #d0d0d0;
$submit-btn-c: #39bebf;

Choose a reason for hiding this comment

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

Do not abbreviate the names of variables to unreadable ones and try to minimally use naming according to the content (that is, if you want to keep the color white, do not make the variable white-color, but name it more universally)

src/index.html Outdated
Comment on lines 380 to 384
<img
src="./images/features-vector-photo.svg"
alt="features vector photo"
class="features__vector-photo"
>

Choose a reason for hiding this comment

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

Don't forget about good code formatting.

<h3 class="features__second-title features__second-title--hidden">
Connectivity
</h3>

Choose a reason for hiding this comment

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

Suggested change

src/index.html Outdated
<div class="features__bottom">
<img
src="./images/features-background.png"
alt="features__background-img"

Choose a reason for hiding this comment

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

The alt attribute contains information that will be displayed if the photo cannot be loaded, as well as voiced by the screen reader, so there should be a description of what the image contains.

src/index.html Outdated
class="features__background-img"
>
</div>

Choose a reason for hiding this comment

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

Suggested change

@OstinM OstinM requested a review from roman-mirzoian October 25, 2023 13:50
Copy link

@ArtemTeslenko ArtemTeslenko left a comment

Choose a reason for hiding this comment

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

There are not fixed issues from previous review, please fix

@OstinM OstinM requested a review from ArtemTeslenko October 26, 2023 09:38
Copy link

@BudnikOleksii BudnikOleksii 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 👍
Just one moment. The form should do the same as in previous tasks, just reset after submissions

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