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 #342

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

Develop #342

wants to merge 11 commits into from

Conversation

oltarasov
Copy link

Copy link

@andriy-shymkiv andriy-shymkiv left a comment

Choose a reason for hiding this comment

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

-background color not like in design:
image

  • layout is jumping when I open/close the menu:
    image
  • give hover for the images like this:
    image
  • what is this? please fix it:
    image
  • the layout is broken here:
    image
  • and it is jumping when I click on imputs:
    image
  • remove this line:
    image
  • on desktop it is broken as well:
    image

Copy link

@nazarmatsevych nazarmatsevych left a comment

Choose a reason for hiding this comment

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

Well done, just minor comments to be fixed:

  1. add scroll-behavior: smooth to make scroll animation smooth

  2. Add type email to email field and make both fields required
    image

  3. Change 2019 to 2023
    image

Copy link

@nazarmatsevych nazarmatsevych left a comment

Choose a reason for hiding this comment

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

The page should not reload on submit, also clear form fields on the button click
image

Here are how you could achieve that
image

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Great work! Let's make it perfect:
Screenshot 2023-10-01 at 16 32 44
Consider fixing the validation error
Screenshot 2023-10-01 at 16 31 41
The image looks skewed
Screenshot 2023-10-01 at 16 31 20
Check font styling here
Screenshot 2023-10-01 at 16 30 21
Space between sections should be added
Screenshot 2023-10-01 at 16 30 02
The user should be able to scroll the menu in the landscape mode
Screenshot 2023-10-01 at 16 28 34
The redundant space should be removed
Screenshot 2023-10-01 at 16 27 32

Screenshot 2023-10-01 at 16 27 01 Check fonts here Screenshot 2023-10-01 at 16 26 46 Titles should be aligned differently to meet the Figma design Screenshot 2023-10-01 at 16 26 30 Add cursor pointer to the interactive elements Screenshot 2023-10-01 at 16 25 35 Screenshot 2023-10-01 at 16 25 56 The elements should be aligned according to the Figma design

Comment on lines +13 to +14
<link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;700&display=swap"
rel="stylesheet">

Choose a reason for hiding this comment

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

Suggested change
<link href="https://fonts.googleapis.com/css2?family=Inter:wght@400;700&display=swap"
rel="stylesheet">
<link
href="https://fonts.googleapis.com/css2?family=Inter:wght@400;700&display=swap"
rel="stylesheet"
>

Consider fixing indentations according to the code style guide

src/index.html Outdated
Comment on lines 114 to 116
<img class="benefits__logo" src="./images/design2.svg"
alt="design2"
>

Choose a reason for hiding this comment

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

Suggested change
<img class="benefits__logo" src="./images/design2.svg"
alt="design2"
>
<img
class="benefits__logo"
src="./images/design2.svg"
alt="design2"
>

src/index.html Outdated
Comment on lines 121 to 123
<img class="benefits__logo" src="./images/design3.svg"
alt="design3"
>

Choose a reason for hiding this comment

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

Suggested change
<img class="benefits__logo" src="./images/design3.svg"
alt="design3"
>
<img
class="benefits__logo"
src="./images/design3.svg"
alt="design3"
>

src/index.html Outdated
Comment on lines 128 to 130
<img class="benefits__logo" src="./images/design4.svg"
alt="design4"
>

Choose a reason for hiding this comment

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

Suggested change
<img class="benefits__logo" src="./images/design4.svg"
alt="design4"
>
<img
class="benefits__logo"
src="./images/design4.svg"
alt="design4"
>

src/index.html Outdated
Comment on lines 137 to 139
<img class="meet-luna__image" src="./images/meetluna.png"
alt="MEETLUNA"
>

Choose a reason for hiding this comment

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

Suggested change
<img class="meet-luna__image" src="./images/meetluna.png"
alt="MEETLUNA"
>
<img
class="meet-luna__image"
src="./images/meetluna.png"
alt="MEETLUNA"
>

src/index.html Outdated
Comment on lines 263 to 265
<img class="footer__link-back-home__image" src="./images/icon/vector.svg"
alt="vect"
>

Choose a reason for hiding this comment

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

Suggested change
<img class="footer__link-back-home__image" src="./images/icon/vector.svg"
alt="vect"
>
<img
class="footer__link-back-home__image"
src="./images/icon/vector.svg"
alt="vect"
>

src/index.html Outdated
Comment on lines 268 to 270
<a class="footer__icon" href="https://www.facebook.com/"
target="_blank"
>

Choose a reason for hiding this comment

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

Suggested change
<a class="footer__icon" href="https://www.facebook.com/"
target="_blank"
>
<a
class="footer__icon"
href="https://www.facebook.com/"
target="_blank"
>

src/index.html Outdated
Comment on lines 273 to 275
<a class="footer__icon" href="https://twitter.com/"
target="_blank"
>

Choose a reason for hiding this comment

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

Suggested change
<a class="footer__icon" href="https://twitter.com/"
target="_blank"
>
<a
class="footer__icon"
href="https://twitter.com/"
target="_blank"
>

src/index.html Outdated
Comment on lines 278 to 280
<a class="footer__icon" href="https://www.instagram.com/"
target="_blank"
>

Choose a reason for hiding this comment

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

Suggested change
<a class="footer__icon" href="https://www.instagram.com/"
target="_blank"
>
<a
class="footer__icon"
href="https://www.instagram.com/"
target="_blank"
>

function showSlide(index) {
slides.forEach((slide, i) => {
// slide.style.display = i === index ? 'block' : 'none';
});

Choose a reason for hiding this comment

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

If you don't need this cycle you can delete it

Copy link

@anastasiiavorobiova anastasiiavorobiova left a comment

Choose a reason for hiding this comment

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

Copy link

@IvanFesenko IvanFesenko left a comment

Choose a reason for hiding this comment

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

@oltarasov oltarasov requested a review from IvanFesenko October 2, 2023 15:10
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.

6 participants