Skip to content

Sheffield | May-2025 | Yousef | Wireframe #421

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Sheffield | May-2025 | Yousef | Wireframe #421

wants to merge 6 commits into from

Conversation

R7Yos
Copy link

@R7Yos R7Yos commented May 10, 2025 •
edited
Loading

Learners, PR Template

Self checklist

  • I have committed my files one by one, on purpose, and for a reason
  • I have titled my PR with REGION | COHORT_NAME | FIRST_NAME LAST_NAME | PROJ_NAME
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

  • Moved the .hero section inside <main> for structural consistency
  • Ensured the first article (README) spans the full width — aligning with the left edge of the second article and right edge of the third
  • Applied flexbox layout to the second and third articles using .flex-container
  • Added CSS to left-align all child elements inside each article (text-align: left)
  • Preserved semantic HTML structure and responsive layout
  • Double check and fix all HTML code Errors.

Questions

I don't understand your comment "I think this should be "README File".".
Could you explain more please.

R7Yos added 3 commits May 10, 2025 16:29
@R7Yos
Added 3 images
a6580a6 
Signed-off-by: R7Yos <[email protected]>
@R7Yos
My Wireframe webpage
0fcaed8 
Signed-off-by: R7Yos <[email protected]>
@R7Yos
Check box
165d4d1 
Signed-off-by: R7Yos <[email protected]>
@netlify Netlify
Copy link

netlify bot commented May 10, 2025 •
edited
Loading

✅ Deploy Preview for cyf-onboarding-module ready!

Name Link
🔨 Latest commit c724a7e
🔍 Latest deploy log https://app.netlify.com/projects/cyf-onboarding-module/deploys/682762087fb1c60008b06653
😎 Deploy Preview https://deploy-preview-421--cyf-onboarding-module.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse		 2 paths audited
Performance: 88 (🔴 down 12 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 91 (🟢 up 5 from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@R7Yos R7Yos added the Needs Review Participant to add when requesting review label May 10, 2025
cjyuan
cjyuan reviewed May 11, 2025
View reviewed changes
Copy link
Contributor

@cjyuan cjyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Can you change the followings so that your page layout can better match the wireframe layout:
  • The left edge of the first article should align with the left edge of the second article, and the right edge should align with the right edge of the third article.
  • The child elements of each article are left aligned (not centered) in the wireframe.

image

  1. Can you give a brief description to your PR?

image

  1. Regarding the PR title:
  • The cohort name in the title should be "May-2025".

Wireframe/index.html
<article id="readme">
<img src="README.FILE.png" alt="README File Image">
<div class="content">
<h2>README.FILE</h2>
Copy link
Contributor

@cjyuan cjyuan May 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"README.FILE" would suggest you are referring to a file named exactly "README.FILE". "README File" would probably be more appropriate.

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels May 11, 2025
@R7Yos R7Yos changed the title Sheffield | ITP2025 | Yousef | Wireframe Sheffield | May-2025 | Yousef | Wireframe May 13, 2025
@R7Yos R7Yos added the Needs Review Participant to add when requesting review label May 13, 2025
@cjyuan
Copy link
Contributor

cjyuan commented May 15, 2025

The changes look good.

I just noticed something that could use some improvement:

  1. When a wireframe is provided, our implementation should closely reflect its appearance and layout to ensure consistency with design expectations. In the Wireframe image, the link "READ MORE" has a border. Can you add a border to the "Read more" links on your page?

  2. The requirement, "The page footer is fixed to the bottom of the viewport", has not yet been made. (Suggestions: Ask ChatGPT what that requirement means)

Note: In addition to implementing changes based on reviewer feedback, it's good practice to respond to each comment, indicating what was changed or how the request was addressed.

@cjyuan cjyuan removed the Needs Review Participant to add when requesting review label May 15, 2025
@R7Yos
fix: add fixed footer and border styling to "Read more" links
c724a7e 
- Fixed footer to bottom of viewport
- Added border and hover effect to "Read more" summary elements
- Styled footer links for better appearance

Signed-off-by: R7Yos <[email protected]>
@R7Yos R7Yos closed this May 16, 2025
@R7Yos R7Yos added Needs Review Participant to add when requesting review and removed Reviewed Volunteer to add when completing a review labels May 16, 2025
@cjyuan
Copy link
Contributor

cjyuan commented May 16, 2025

Why do you close this PR?

@R7Yos R7Yos reopened this May 16, 2025
@R7Yos
Copy link
Author

R7Yos commented May 16, 2025 via email

@cjyuan
Copy link
Contributor

cjyuan commented May 16, 2025

Changes look good! Well done.

@cjyuan cjyuan added Complete Volunteer to add when work is complete and review comments have been addressed and removed Needs Review Participant to add when requesting review labels May 16, 2025
@R7Yos R7Yos mentioned this pull request May 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjyuan cjyuan cjyuan left review comments

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Complete Volunteer to add when work is complete and review comments have been addressed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@R7Yos @cjyuan