Desktop version #6
Conversation
- it adds nav-bar menu - update self-presentation and contact me responsiveness
- language list response well - Role element response well
There was a problem hiding this comment.
STATUS: REQUIRED CHANGES ♻️♻️
Hi @kusiLaw
Good job so far! I know you worked hard to implement this project, I really appreciate the effort you put in! But there are some issues that you still need to work on to go to the next project but you are almost there!
To highlight 🍾
- All linters are passing ✔️
- Good commit history ✔️
- Your design looks good 💯 ✔️
- Good PR description ✔️
-
- Good Design ✔️
- Follow 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.
Cheers and Happy coding!👏👏👏
Feel free to leave any questions or comments in the PR thread if something is not 100% clear.
Please ping me @tresorsawasawa when you comment so I can receive the notification or use slack with the same name.
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
| <div class="contact-forms footer-items"> | ||
| <form class="message-form" action="https://formspree.io/f/1" method="post"> | ||
| <ul> | ||
| <li class="input-li"> |
There was a problem hiding this comment.
-
You are almost there with your styling. I just noticed that you added you should remove the border on the
Get In Touchbutton to stick to the Figma design. You can only add this property(border: none;) in the button styling to fix the issue. -
[ OPTIONAL ] RECOMENDED: I think it would be better if you use the text
Write your messages hereas aplaceholder="..."as for now it requires the user to every time deleted the text itself before he writes his message. I know you can do that 💪 💪.
- form button rename "git it touch" to "Git in Touch" - form button border removed - form text area has placeholder
There was a problem hiding this comment.
Hi @kusiLaw 👋 ,
Great work on making the changes requested by a previous reviewer 👏
You've done well implementing some of the requested changes, but there are still some which aren't addressed yet.
Highlights
✔️ Nice UI
✔️ Descriptive PR title
✔️ Good use of Github flow
Required Changes ♻️
Check the comments under the review.
Optional suggestions
-
[ OPTIONAL ] It would be great if you could add
pointer: cursor;rule to all your buttons for better user experience. -
[ OPTIONAL ] It would be great if you can make the size of these badges the same regardless of the text content.
-
[ OPTIONAL ] It would be great if you could vertically align the social media icons. Besides, kindly add appropriate url to the anchor tags if you already have accounts created.
-
[ OPTIONAL] I see that you have used different css classes to change the order of project images on alternating project cards. Can you try to achieve the same, but use the same CSS classes using
Flex: orderandnth childcss selector? For example,
<section class="works">
<article class="card">
<img src="">
<div class="project-details">.....</div>
</article>
<article class="card">
<img src="">
<div class="project-details">.....</div>
</article>
<article class="card">
<img src="">
<div class="project-details">.....</div>
</article>
</section>
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 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.
| </li> | ||
| <li class="submit-li"> | ||
| <button type="submit" id="getInTouch">Get it Touch</button> | ||
| <button type="submit" id="getInTouch">Get in touch</button> |
There was a problem hiding this comment.
- Kindly remove the black border around the
Get in touchbutton as suggested by the previous reviewer.
You can add theborder: none;rule in.message-form ul li buttonselector in the mobile version so that it will be applied to all screen sizes.
- form button border removed both mobile and desktop version - social media icon arrange well - languages used has same width - buttons uses point upon hover
There was a problem hiding this comment.
Hi @kusiLaw ,
Your project is complete! There is nothing else to say other than... it's time to merge it 
Congratulations! 🎉
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.
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.
This pull request adds to the desktop version of my portfolio