Portfolio Desktop design #5
Conversation
There was a problem hiding this comment.
Changes Required ♻️
Hi @ichala,
Nice job, you are almost there.
(Highlights) Good Points: 👍
- You documented your work professionally. ✔
- Github flow is used correctly. ✔
- All linters checks are ok. ✔
(Changes Required) Aspects to improve: ♻️
- Please check the inline comments below.
[Optional] suggestions:
- Nothing to mention.
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, and 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.
styles.css
Outdated
| @@ -418,3 +426,469 @@ body { | |||
| margin-bottom: 20px; | |||
| } | |||
| } | |||
|
|
|||
| @media only screen and (min-width: 768px) { | |||
There was a problem hiding this comment.
- Please change the value of
min-widthany value between 992 and 1100 is acceptable because this is the desktop version, not the tablet version.
for examplemin-width: 1080px
styles.css
Outdated
|
|
||
| @media only screen and (max-device-width: 480px) { |
There was a problem hiding this comment.
- Please remove the media query
max-widthbecause you already developing a mobile-first application and there is no benefit from it contrariwise it makes you repeat a lot of code again in the desktop version and as you learn your code has to be dry.
styles.css
Outdated
| .form-flex { | ||
| width: 100%; | ||
| display: flex; | ||
| flex-direction: column; | ||
| justify-content: flex-start; | ||
| align-items: flex-start; | ||
| } |
There was a problem hiding this comment.
- Please add the right width for The form and the heading text beside it and edit the fint size and wieght for that text too. and try to stick as much as possible to the given Figma design.
| Figma Design | Your Design |
|---|---|
 |
 |
styles.css
Outdated
| .bg-about { | ||
| background-image: url(images/bg-abt.png); | ||
| background-size: 20%; | ||
| background-repeat: no-repeat; | ||
| background-position: top right; | ||
| } |
There was a problem hiding this comment.
- I can see in the about me section that the text is overlapping the background. Please fix that and try to stick as much as possible to the given Figma design.
| Figma Design | Your Design |
|---|---|
 |
 |
There was a problem hiding this comment.
Hi @ichala
Great job implementing the recommended changes by the last reviwer.
Your project is complete! There is nothing else to say other than... it's time to merge it 
STATUS: APPROVED 🟢
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 @Whoistolu 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 PR contains the following features :