-
-
Notifications
You must be signed in to change notification settings - Fork 85
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
ITP,JAN25,LONDON | GENET_HAILE | MODEL ONBOARDING WIREFRAM | WEEK 1 #249
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@halilibrahimcelik can you please review my work please. thank you |
Wireframe/index.html
Outdated
<p> | ||
Lorem ipsum dolor sit amet consectetur adipisicing elit. Quisquam, | ||
voluptates. Quisquam, voluptates. | ||
|
||
</p> | ||
<a href="">Read more</a> |
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 should put link inside of href
</p> | ||
</header> | ||
<main> | ||
<article> | ||
<img src="placeholder.svg" alt="" /> | ||
<h2>Title</h2> | ||
<img src=https://miro.medium.com/v2/resize:fit:1200/1*oppPsIqwEiB_kndtd6GWxA.png alt="" /> |
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 you tell me why we should always altx texts for images?
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.
To improve accessibility by describing what an image is showing to visitors who do not have the ability to see them
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.
So we always fill tag attributes according to visuals we use in our images
Wireframe/index.html
Outdated
project management. | ||
</p> | ||
<a href="">Read more</a> | ||
</main> | ||
<footer> | ||
<p> | ||
This is the default, provided code and no changes have been made yet. |
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 should add some styling to your footer which allows user to distinguish where the footer starts and ends
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 you please check my work when you can. Thank you.
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.
I can't see any changes, Could you also look my other feedbacks as well
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.
Still don't see any changes, are you sure you committed your recent changes to your PR ?
@@ -27,11 +27,11 @@ <h2>README file</h2> | |||
<p> | |||
|
|||
</p> | |||
<a href="">Read more</a> | |||
<a href="https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-readmes">Read more</a> |
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.
If you're using internal website links in your website, I suggest you to use target="_blank"
in a tag as well, which will allow user to open link in another window
Learners, PR Template
Self checklist
Changelist
Briefly explain your PR.
Questions
Ask any questions you have for your reviewer.