ITP_JAN_25 / YORSALEM MESMER / Module-Onboarding / Week 1 #222
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Jerry-yorsalem
changed the title
|
When you committing your changes, please try to be as clear as possible while naming your changes in your commit. Also do not push all changes with one single commit, try to divide your changes into small blocks. This way it will be more easy for reviews to inspect changes. |
|
Also please add "Need Reviews" labels when you create a PR. |
halilibrahimcelik
added
the
Review in progress
|
Your accessibility score needs to be 100, please inspect outcome of lighthouse and make little more research to increase the score. |
Wireframe/index.html
Outdated
|
|
||
| <main> | ||
| <article> | ||
| <img src="https://media.geeksforgeeks.org/wp-content/uploads/20210915020701/Readme1.png" alt="https://media.geeksforgeeks.org/wp-content/uploads/20210915020701/Readme1.png" /> |
There was a problem hiding this comment.
You should write a descriptive text inside of alt attribute instead of using an url
There was a problem hiding this comment.
I changed the url file to a descriptive text inside the alt attribute. Thank you for the advice.
| </main> | ||
| <footer> | ||
| <p>Creat by Yorsalem Mesmer</p> | ||
| </footer> |
There was a problem hiding this comment.
footer's position fixed okay but user does not really understand what that part is really about, it would be nice to show your footer area with e.i background color
There was a problem hiding this comment.
I added a background colour to the footer.
Wireframe/index.html
Outdated
| Using branches effectively helps keep your project organized and ensures that the main codebase remains stable. | ||
| </p> | ||
| <a href="">Read more</a> | ||
| </div> |
There was a problem hiding this comment.
why these read more buttons did not direct me to different page ?
There was a problem hiding this comment.
Because I didn't attached my link to href attribute. Sorry about that I though I did.
| @@ -50,6 +50,7 @@ main { | |||
| margin: auto; | |||
| } | |||
| footer { | |||
| background-color: rgb(184, 134, 249); | |||
| position: fixed; | |||
There was a problem hiding this comment.
as best practice footers are generally stretched to its container, this is a design enhancement, you don't have to do it, but it is better to practice more which will improve your UI and UX skills
halilibrahimcelik
added
Reviewed
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.