Feature branch #1
Conversation
There was a problem hiding this comment.
Hi @ikoote1,
♻️ Required Changes ♻️
Good job so far!
There are some issues that you still need to work on to go to the next project but you are almost there!
Highlights
- No linter errors ✔️
- Correct Github flow ✔️
Required Changes ♻️
Check the comments under the review.
Optional suggestions
Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.
You can also consider:
- [OPTIONAL] Kindly add a descriptive PR with a tile and a summary of the actions for professional documentation. 👇🏽
edit.PR.mp4
- Please 🙏🏽 add the
.pngand.svgfiles into animagesfolder to be organized.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
style.css
Outdated
| .toolbar { | ||
| position: absolute; | ||
| width: 375px; | ||
| height: 92px; |
There was a problem hiding this comment.
- Kindly correct the dimension of the toolbar, it is 👉🏽
height: 46px;👈🏽 don't consider the status bar because it is a reference.
style.css
Outdated
| font-size: 56px; | ||
| line-height: 64px; | ||
| letter-spacing: 0.37px; | ||
| color: red; |
There was a problem hiding this comment.
- Please 🙏🏽 correct the color, it is
color: #FF6B00;instead ofredto be similar to the Figma template.
| background: #1c1a19; | ||
| } | ||
|
|
||
| h1 { |
There was a problem hiding this comment.
- Kindly correct the spaces (margins) between the title and subtitle to be similar to the Figma template 👇🏽
| Figma template | Your design |
|---|---|
 |
 |
style.css
Outdated
| .micro7 { | ||
| position: absolute; | ||
| left: 0.04%; | ||
| right: 0.03%; | ||
| top: 70%; | ||
| bottom: 0%; | ||
| } |
There was a problem hiding this comment.
- Please 🙏🏽 correct the alignment position of social icons to be similar to the Figma template, it could be better to use pixels instead of percentages.
| Figma template | Your design |
|---|---|
|
|
index.html
Outdated
| <header> | ||
| <nav class="toolbar"> | ||
| <nav class="tbar"> | ||
| <nav class="menu-bar"> | ||
| <img src="John Doe.png" class="john-doe"> | ||
| <img src="Icon - Menu.png" class="icon-Menu"> | ||
| </nav> | ||
| </nav> |
There was a problem hiding this comment.
- For good practices of HTML&CSS the
headertag should close after the navbar. Please 🙏🏽 fix it.
index.html
Outdated
| <nav class="toolbar"> | ||
| <nav class="tbar"> | ||
| <nav class="menu-bar"> | ||
| <img src="John Doe.png" class="john-doe"> |
There was a problem hiding this comment.
- Kindly add an
atag to the logo this is for three reasons: convention, semantics, and user experience(UX). Check this link for more info. Also, we can use our name to personalize our portfolio.
index.html
Outdated
| </nav> | ||
| </nav> | ||
|
|
||
| <nav class="headline"> |
There was a problem hiding this comment.
- The correct semantic for this section is the
sectiontag, please 🙏🏽 fix it for the good practices of HTML&CSS.
index.html
Outdated
| </nav> | ||
|
|
||
| <nav class="headline"> | ||
| <nav> |
There was a problem hiding this comment.
- If you want to separate elements in a container you could use
divtag because this is not a navbar. Please 🙏🏽 fix it for good practice of the HTML&CSS.
index.html
Outdated
| <p class="txt">I can help you build a product , feature or website Look through some of my work and experience! If you like what you see and have a project you need coded, | ||
| don’t hestiate to contact me.</p> | ||
| </nav> | ||
| <img src="Illustration.svg" class="illust" > |
There was a problem hiding this comment.
- Please 🙏🏽 use CSS background if an image is not part of the content (aesthetic reasons) for good practice of HTML&CSS. Check this link for more info.If an image is not part of the content (aesthetic reasons) use CSS background-image. Check this link for more info.
index.html
Outdated
| don’t hestiate to contact me.</p> | ||
| </nav> | ||
| <img src="Illustration.svg" class="illust" > | ||
| <img src="Social Media.png" class="micro7"> |
There was a problem hiding this comment.
- Kindly for the list of icons use a
ultag to group them, andliper icon; each icon has a link (atag). Apart from being semantically correct, theatag will let you connect your social networks with the icons, usinghref. - [OPTIONAL] Please 🙏🏽 link the social icons to the web pages because you will need them later 👇🏽
- GitHub
- Angellist
- Medium
There was a problem hiding this comment.
Hi @ikoote1 👋,
Great work on making the changes requested by a previous reviewer 👏
Highlights
- All linters checks passed ✔️
- README.md file is professional ✔️
You've done well implementing some of the requested changes, but there are still some which aren't been addressed yet.
Required Changes ♻️
Kindly check the comments under the review.
Optional suggestions
- Kindly consider the suggestion tagged [OPTIONAL]
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please, remember to tag me in your question so I can receive the notification. You can tag me by typing @Gambit142
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
index.html
Outdated
| <header> | ||
| <section class="toolbar"> | ||
| <div class="tbar"> | ||
| <div class="menu-bar"> |
There was a problem hiding this comment.
- For semantic purposes, kindly change this tag from a
divto anavtag. I noticed it was anavtag before then you changed it to adivtag. I am referring to thedivtag with theclassattributemenu-bar👍
index.html
Outdated
| <section class="toolbar"> | ||
| <div class="tbar"> | ||
| <div class="menu-bar"> | ||
| <p class="john-doe">IKOOTE</p> |
There was a problem hiding this comment.
- For semantic purposes, kindly change this from a
ptag to anatag. Making the logo a link improves user experience and makes the navigation of the webpage easier. It is also good HTML practice to make the logo of a webpage a link. 👍
index.html
Outdated
| <header> | ||
| <section class="toolbar"> |
There was a problem hiding this comment.
- Just like the first reviewer mentioned, the
sectiontag should not be inside aheaderbecause theheadertag is a section on its own. I would advise you to change this so that it becomes semantic. 👍
Hint:
<header>
<a href="#">logo</a>
<nav>hamburger button</nav>
</header>| .toolbar { | ||
| position: absolute; | ||
| width: 375px; | ||
| height: 46px; | ||
| left: 0; | ||
| top: 0; | ||
| background: #3c3a39; | ||
| } |
There was a problem hiding this comment.
- [OPTIONAL] Based on the requirements, this section should be aligned using a flexbox. Using flexbox makes your design responsive across various screen sizes. I would advise you to implement this suggestion. 👍
index.html
Outdated
| <p style="background-image: url('./images/Illustration.svg');" class="illust"> | ||
|
|
There was a problem hiding this comment.
- Great work making the changes requested by the previous reviewer. However, it is not a good HTML/CSS practice to use inline styles when making CSS declarations. I would advise you to put this declaration in your style sheet. For more information, you can read this article
index.html
Outdated
| <ul> | ||
| <li><img src="./images/Vector1.png" alt="github"></li> | ||
| <li><img src="./images/Vector2.png" alt="instagram"></li> | ||
| <li><img src="./images/Vector3.png" alt="tree"></li> | ||
| <li><img src="./images/Vector4.png" alt="twitter"></li> | ||
| <li><img src="./images/Vector5.png" alt="messenger"></li> | ||
| </ul> |
There was a problem hiding this comment.
- Great work making the changes requested by the previous reviewer. However, you forgot to wrap each image in the
atag. This is the idea, when I click each icon, it should direct me to the social media page of that link. Kindly wrap each image in theatag. 👍
Hint:
<ul>
<li><a href="..."><img src="./images/Vector1.png" alt="github"></a></li>
...
</ul>
There was a problem hiding this comment.
Hi @ikoote1 ,
Good job so far!
May I highlight the following:
- The repo looks professional.:heavy_check_mark:
- There are no linter errors:heavy_check_mark:
- Your project looks awesome:heavy_check_mark:
STATUS: APPROVED 💪👏
Congratulations! 🎉
Your project is complete! It's time to merge it 
Optional suggestions
- Please check the comment under the review.
Every comment with the [OPTIONAL] prefix won't stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better. Some of them were simply missed by the previous reviewer and addressing them will really improve your application.
Cheers and Happy coding! 👏 👏 👏
Feel free to leave any questions or comments in the PR thread and tag me @algerina if something is not 100% clear.
Please, do not open a new Pull Request for re-reviews. You should use the same Pull Request submitted for the first review, either valid or invalid unless it is requested otherwise.
As described in the Code reviews limits policy you have a limited number of reviews per project (check the exact number in your Dashboard). If you think that the code review was not fair, you can request a second opinion using this form.
If anything seems to be unclear to you, reach out and ask anything by commenting on this Pull Request and mention me as Algerina, I will receive a notification and will respond.
Reviewed by Amel Khiri
Added CSS and HTML files with the toolbar and headline sections