-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
base: master
Are you sure you want to change the base?
Add task solution #2662
Conversation
There was a problem hiding this 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(
There was a problem hiding this 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 ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/index.html
Outdated
|
||
<h4 class="title title--article">Sporty 4</h4> | ||
|
||
<p class="main__paragraph"> |
There was a problem hiding this comment.
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
src/images/photos/bike-1.jpg
Outdated
There was a problem hiding this comment.
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
src/styles/blocks/title.scss
Outdated
color: #fff; | ||
|
||
&--secondary { | ||
font-family: Poppins, Helvetica, sans-serif; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this 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:
There was a problem hiding this 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
There was a problem hiding this 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.
DEMO LINK