-
-
Notifications
You must be signed in to change notification settings - Fork 349
London | 25-ITP-September | Carlos Abreu | Sprint 1 | Wireframe #906
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
✅ Deploy Preview for cyf-onboarding-module ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Hey dude, it's Khogan here👋, I saw you were waiting for your pull requests to be reviewed, have you done everything on here? - https://cyf-programming.netlify.app/onboarding/success/? Even if the volunteers haven't completed your code review I'm just recommending you submit the pull requests/your coursework on the CYF course portal ASAP: https://application-process.codeyourfuture.io/ |
|
Please improve your PR and code according to the PR Guide first. |
…howing the two last queries (wireframe and git branch) side by side
|
@cjyuan and @jenny-alexander I fixed the mistake and I think it's working as requirement. |
Can you address these issues? Have you tried feeding your code to AI for a second opinion? |
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.
@carlosyabreu - Your PR is almost complete but there are few things missing. I left a few comments for you to review.
(Sorry for not getting back to you earlier - I am working full time during the day & sometimes I don't have time at night either because of family obligations). 😄
Wireframe/index.html
Outdated
| <div class="top"> | ||
| <section class="article"> | ||
| <article> | ||
| <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.
This is the default image - can you update it with an image related to a readme article? The alt tag should have info (for a screen reader).
| </div> | ||
| </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 state that it should be 'fixed'. This means that no matter where you scrolls on the page (up, down), the footer will always be visible. Can you make this change?
Wireframe/index.html
Outdated
| <p> | ||
| The readme file is used to explain what are the files uploaded and how it can be installed or used. It allows the owner to upload and add images and videos to help the reader navigate through the project. A well-written readme file is helpful for a new user or developer to get a good understanding about the project, its structure and attract more participants to add new features. | ||
| </p> | ||
| <a href="https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/customizing-your-repository/about-readmes" target="_blank">Read more about README and its purpose.</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.
I noticed the button labels are a bit different from the wireframe.
Can you review the wireframe requirements here?
https://github.com/CodeYourFuture/Module-Onboarding/tree/main/Wireframe#wireframe
Notice the name of the button? It says 'Read More'.
🌱 Following the wireframe closely shows attention to detail and that you can implement a design as specified!
|
@jenny-alexander @cjyuan |
|
@cjyuan & @jenny-alexander |
cjyuan
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.
According to https://validator.w3.org/, there are still errors in your HTML code. Can you fix all the errors and warnings in your HTML code?
Wireframe/index.html
Outdated
| <p> | ||
| A branch in Git allows developers to work on new features or fixes separately from the main codebase. This makes collaboration easier and ensures the main project stays stable while changes are tested. | ||
| </br> | ||
| In other words, it could be say a Git branch is an independent line of development that allows developers to work on new features, fixes, or experiments separately from the main codebase without affecting the stable version. | ||
| </br> | ||
| Branches enable collaboration, experimentation, and version control by letting multiple developers work simultaneously. | ||
| </p> |
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.
Why use <br>?
If "A branch ..." and "In other words ..." are two separate paragraphs, there is a more appropriate way to encode them as separate paragraphs. If it is a single paragraph, the text might not flow naturally on some screen width.
Wireframe/index.html
Outdated
| <p> | ||
| The readme file is used to explain what are the files uploaded and how it can be installed or used. It allows the owner to upload and add images and videos to help the reader navigate through the project. A well-written readme file is helpful for a new user or developer to get a good understanding about the project, its structure and attract more participants to add new features. | ||
| </p> |
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.
Lines 22-24 can be better formatted as:
<p>
The readme file is used to explain what are the files uploaded and how it can
be installed or used. It allows the owner to upload and add images and videos
to help the reader navigate through the project. A well-written readme file is
helpful for a new user or developer to get a good understanding about the
project, its structure and attract more participants to add new features.
</p>
To understand why, you can ask ChatGPT these questions:
- How HTML treats mutliple whitespace characters in text?
- What's the advantage of not writing a long paragraph of text in a single line in HTML?
VSCode's "Format Document" feature can help us format our code for better readability and consistency, including breaking a long line of text into shorter lines of text.
To use the feature, right-click inside the code editor and select the option.
Please note that if there are syntax errors in the code, the "Prettier" extension may not format HTML code properly.
…priate in the context
|
@cjyuan A README file explains the contents of a project and how to install or use it. It often includes images, videos, and instructions to help readers navigate the project easily. A well-written README helps new users or developers understand the project's purpose and structure, and can attract more contributors to add new features. Again thanks. |
|
@cjyuan |
|
Good job but that only addressed one of my comments. Can you address ALL comments? |
|
@cjyuan |
|
Changes look good. Well done. |



Learners, PR Template
Self checklist
Changelist
@jenny-alexander
This PR is about the Wireframe project that needs to be included into CYF main project.
I want to add I've deleted the repo you reviewed before, create a new repo that I made a mistake again merging my local feature/wireframe branch with my local main then I push to main GitHub branch.
After talking with another reviewer on #cyf-code-review channel I was advice not to merge in any circumstance my local branch (feature/wireframe) with local repo main branch.
Then I delete again the repo (2nd time) and started it clean.
This repo is the last one and the one I want to be reviewed.
I add the deadline is today as I need it to be complete so I can apply for next stage called trainee.
Thank you for your understanding.
Questions
I think after deleting to time the repo now the PR is correct.