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

Add task solution #2662

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

Conversation

Mykyta-snacj
Copy link

@Mykyta-snacj Mykyta-snacj changed the title positioning header Add taka solution Jul 31, 2024
Copy link

@BogdanMaliuta BogdanMaliuta left a comment

Choose a reason for hiding this comment

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

Can`t review without demo(

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

great start just use better image ;)

Choose a reason for hiding this comment

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

this image has low quality
you should import this whole picture and then adjust it's position
image

@Mykyta-snacj Mykyta-snacj changed the title Add taka solution Add task solution Aug 1, 2024
Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

  • add all needed spaces between blocks of text e.g
    image

  • change position of background
    image

src/index.html Outdated

<h4 class="title title--article">Sporty 4</h4>

<p class="main__paragraph">

Choose a reason for hiding this comment

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

this element is inside article block

Choose a reason for hiding this comment

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

all of images should be in better quality

color: #fff;

&--secondary {
font-family: Poppins, Helvetica, sans-serif;

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 repeat styles from title

Copy link

@BogdanMaliuta BogdanMaliuta left a comment

Choose a reason for hiding this comment

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

Here is no header
image
Look at demo something going wrong with styles, fix it please

@Mykyta-snacj
Copy link
Author

Here is no header image Look at demo something going wrong with styles, fix it please

This is a mobile version. Reduce the width to 320px and scroll the page.

Copy link

@natalia-klonowska natalia-klonowska 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 🚀 almost done :D really nice that you use mixins but you should also create some variables for e.g colors. Additionally there are some functionality that you still need to add. All of them are mentioned in checklist:
image

  • add some hover effect on images (e.g increase)
  • form shouldn't be submitted if some of fields are empty (suggest what's needed)
  • phone number, email and address should be clickable and change color on hover.
    image

Copy link

@natalia-klonowska natalia-klonowska left a comment

Choose a reason for hiding this comment

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

when form is properly filled out the send button should be interactive. now nothing changes so you should add something to indicate that form was submitted such as clearing all inputs. Also change cursor to a pointer when hovering over the button

Copy link

@Zibi95 Zibi95 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! I see that you fixed the issues found by Natalia. I didn't find anything that would cause this task to fail. LGTM! However, consider minifying the images on the page and using srcset to ensure the images adjust properly to different screen sizes. Remember, this project will probably end up in your portfolio, so make sure the code is clean and free of unnecessary comments.

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