-
-
Notifications
You must be signed in to change notification settings - Fork 335
Glasgow | 25-ITP-SEP | Chibuikem Okwu | Sprint 1 | Wireframe #909
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
base: main
Are you sure you want to change the base?
Conversation
based on DevTools and Lighthouse feedback
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
5 similar comments
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
to collect name, email, colour and size for a T-shirt order.
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
Wireframe/index.html
Outdated
| <main> | ||
| <article> | ||
| <div> | ||
| <img src="placeholder.svg" 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.
All 3 articles should have an image that relates to the article. Right now each image is using:
<img src="placeholder.svg" alt="" />
This is just a placeholder and you should replace with a real image instead.
Wireframe/index.html
Outdated
| href="https://www.makeareadme.com/" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| >Learn more about README files</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.
Can you review the wireframe image again?
https://github.com/CodeYourFuture/Module-Onboarding/tree/main/Wireframe#wireframe
Check carefully and see what each article button should be named.
🌱 It's important to follow the wireframe as much as possible.
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.
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.
The title and description match the wireframe now.
| </section> | ||
| </main> | ||
|
|
||
| <footer> |
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.
The footer requirements are:
The page footer is fixed to the bottom of the viewport.
Do you know what 'fixed to the bottom of the viewport' means? It means that the footer is always visible no matter where you scroll to on the page. Right now, when I scroll to the top of the page, the footer disappears.
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.
Good job updating your footer.
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
1 similar comment
|
Your PR description contained template fields which weren't filled in. Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed. If this PR is not coursework, please add the NotCoursework label (and message on Slack in #cyf-curriculum or it will probably not be noticed). |
and applied needs review.
jenny-alexander
left a comment
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.
The wireframe page looks very close to the requirements now. I left a few comments for you to review.
Wireframe/index.html
Outdated
| <img src="placeholder.svg" alt="" /> | ||
| <h2>Title</h2> | ||
| <div> | ||
| <img src="readme.jpg" 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.
Your images now line up nicely with the article's subject. However, I see that the alt tag doesn't have any information. Can you update this? The alt tag is crucial for a screen reader to be able to read out the image description to a blind or low-vision person.
| </div> | ||
| <br> | ||
| </article> | ||
|
|
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.
The bottom two article's titles don't quite line up horizontally. Can you review and ensure that their alignment matches the wireframe.
Wireframe requirements: https://github.com/CodeYourFuture/Module-Onboarding/tree/main/Wireframe#wireframe
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.
The alignment is still not quite right. Can you find a way to fix it? Perhaps you can research online how to do this with CSS?
of images in the HTML file.
to the left but it didn't work. I tried different ways but nothing worked. this is what I came up with.
| <img src="placeholder.svg" alt="" /> | ||
| <h2>Title</h2> | ||
| <div> | ||
| <img src="readme.jpg" alt=Screenshot of a README file example showing project documentation" /> |
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.
There is a typo in your <img> tag. Can you find it and fix it?
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.
@chibu0070 - the typo still exists on line 21 above. Can you find it within the 'alt' portion of the img tag?
| </div> | ||
| <br> | ||
| </article> | ||
|
|
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.
The alignment is still not quite right. Can you find a way to fix it? Perhaps you can research online how to do this with CSS?
deleted aseond <body> tag
|
@chibu0070 I reviewed your PR again and the 2 issues from a few week's ago are still there. Can you review and fix them?
|


restarted and triedtomake it follow style guide <!--
You must title your PR like this:
Region | Cohort | FirstName LastName | Sprint | Assignment Title
For example,
London | 25-ITP-May | Carol Owen | Sprint 1 | Alarm Clock
Fill in the template below - remove any sections that don't apply.
Complete the self checklist - replace each empty box in the checklist [ ] with a [x].
Add the label "Needs Review" and you will get review.
Respond to volunteer reviews until the volunteer marks it as "Complete".
-->
Learners, PR Template
Self checklist