-
-
Notifications
You must be signed in to change notification settings - Fork 381
London | 26-ITP | Abdul Moiz | Sprint 1 | Wireframe #938
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
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Added Needs Review Label |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Undid changes in /Form-Controls |
|
Hey @A-Moiz! Thanks for submitting a PR for the wireframe exercise. I noticed a few things to change:
|
|
Hi, @jenny-alexander I made the changes you mentioned, let me know if everything is all good now or if anymore changes are needed, thanks. |
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.
Thanks for making your changes directly within the index.html file.
After reviewing your PR again, I see opportunities for improvement.
- Can you remove the placeholder image and insert an image that is related to the article?
- Can you make sure that when a user clicks on the 'Read More' button, they are brought to a webpage/article with information about the article?
I left a few other comments for you to address.
Wireframe/index.html
Outdated
| <p> | ||
| This is the default, provided code and no changes have been made yet. | ||
| </p> | ||
| <header style="text-align: center;"> |
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 exercise requirements concerning where the styling should be done?
https://github.com/CodeYourFuture/Module-Onboarding/tree/main/Wireframe#acceptance-criteria
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.
Hi @jenny-alexander , I added links to relevant articles alongside relevant images.
Wireframe/index.html
Outdated
| <!-- Article 3: Question 4 --> | ||
| <article> | ||
| <img src="placeholder.svg" alt="Placeholder image" /> | ||
| <h2>4. What is the point of the article tag in HTML?</h2> |
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 assignment requirements? I only see 3 questions that need to be answered, but you mentioned a 4th question about the article tag that isn't in the instructions.
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.
Hi, I removed that extra article. I must've mistaken it for another task by accident.
Wireframe/index.html
Outdated
| <p> | ||
| This is the default, provided code and no changes have been made yet. | ||
| </p> | ||
| <footer style="width: 100%; border: solid black 2px;"> |
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.
For the footer section:
- Can you review the exercise requirements concerning where the styling should be done (CSS or in index.html file)?
https://github.com/CodeYourFuture/Module-Onboarding/tree/main/Wireframe#acceptance-criteria - Can you review the appearance of the footer? Right now, it's hard to read the text of the footer because there isn't a background color to the footer. Perhaps you can find a different way to style 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.
Hi, I was 100% sure on what you means by the appearance of the Footer but to be sure I added a background colour of white and changed the font weight of the text.
|
Updated index.html and style.css based on comments received. |
|
All requested changes have been made nicely @A-Moiz - good work! |


Learners, PR Template
Self checklist
Changelist
Questions
N/A