Skip to content

LONDON | ITP MAY-2025 | Hibo Sharif | Form control | Week2 #438

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

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

hibosharif202504
Copy link

@hibosharif202504 hibosharif202504 commented May 12, 2025

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

I have made number of changes to the file to add:
1- The customer's name.
2- The customer's email. The customer email must be valid.
3-Add three choices of t-shirt colour.
4- Give options for size does that the customer want. It must be the following 6 options : XS, S, M, L, XL, XXL

Questions

Please would you review my changes.

The name
The client Email
The choice of three colours
The choices of Tshirt sizes

	modified:   Form-Controls/index.html
Copy link

netlify bot commented May 12, 2025

Deploy Preview for cyf-onboarding-module ready!

Name Link
🔨 Latest commit 4b0493f
🔍 Latest deploy log https://app.netlify.com/projects/cyf-onboarding-module/deploys/682f073c73039200080f27f7
😎 Deploy Preview https://deploy-preview-438--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: 99 (🔴 down 1 from production)
Accessibility: 100 (no change from production)
Best Practices: 100 (no change from production)
SEO: 86 (no change from production)
PWA: -
View the detailed breakdown and full score reports

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

@hibosharif202504 hibosharif202504 changed the title LONDON | MAY-2025 | Hibo Sharif | ITP | Week2 LONDON | MAY-2025 | Hibo Sharif | ITP | Form control | Week2 May 12, 2025
@hibosharif202504 hibosharif202504 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label May 12, 2025
@hibosharif202504 hibosharif202504 requested review from jenny-alexander, SallyMcGrath and AdnaanA and removed request for SallyMcGrath May 13, 2025 18:05
@AdnaanA AdnaanA removed their request for review May 17, 2025 13:35
@hibosharif202504 hibosharif202504 changed the title LONDON | MAY-2025 | Hibo Sharif | ITP | Form control | Week2 LONDON | ITP MAY-2025 | Hibo Sharif | Form control | Week2 May 17, 2025
@hibosharif202504 hibosharif202504 removed the request for review from jenny-alexander May 17, 2025 16:06
@cjyuan
Copy link
Contributor

cjyuan commented May 20, 2025

A well-prepared PR makes it easier for reviewers to approve it with minimal back-and-forth.
The "PR Essentials" file on the Canvas in the #cyf-code-review channel has some helpful tips to make your PRs more robust and ready for review.
Can you take a few minutes to check against the essentials, including how to prepare PR description? Doing so can help speed up the review process.

And as a practice to get better at using AI tools, try running your code by ChatGPT. You can ask ChatGPT

review HTML code:
...        <--- paste your HTML code here

Then take a look at the suggestions it gives you, and pick out the ones that actually make sense for your project. It's a great way to learn and improve your work.

Once you've finished making changes, please leave a comment on this PR so I’ll get notified. Thanks!

@cjyuan cjyuan added Reviewed Volunteer to add when completing a review with trainee action still to take. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels May 20, 2025
[Hibo Sharif] added 3 commits May 21, 2025 14:12
…on the HTML using the W3C Markup validation service.

- I indented the file to make it easier to read.
- I have used the AI tool to modify the code.
@hibosharif202504
Copy link
Author

Hi @cjyuan ,

Thank you for the review.. I have followed the guide published on the Canvas and checked my work using the W3C HTML validation tool. I have also checked the code using the AI tool and made some adjustment to the code.
I have made some adjustment to the PR description.

Many thanks.

@hibosharif202504 hibosharif202504 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label May 21, 2025
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.

Code is free of errors and well indented. Good job!

Can you address the two requests I mentioned in the code?

<!--
try writing out the requirements first as comments
this will also help you fill in your PR message later-->
<div style="margin-bottom: 20px">
Copy link
Contributor

Choose a reason for hiding this comment

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

The spec does says, "No CSS" (including inline CSS).
Can you update your implementation accordingly? You may want to check your lighthouse score after you remove the CSS.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @cjyuan ,
I have removed the CSS codes form the file and I have used lighthouse to check the score of the webpage. Bellow is a link to the score page:

https://pagespeed.web.dev/analysis/https-deploy-preview-438--cyf-onboarding-module-netlify-app-form-controls/ii5r0cohyj?form_factor=desktop

lighthouse-score

name="name"
id="name"
required
pattern="[A-Za-z\s]{2,50}"
Copy link
Contributor

Choose a reason for hiding this comment

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

With this pattern, a user can enter a name containing only space characters. Can you enforce a stricter rule to deny any name containing only space characters?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @cjyuan
I have updated the code to enforce the rule of no name only containing space characters.

@cjyuan cjyuan removed the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label May 21, 2025
@hibosharif202504 hibosharif202504 added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label May 22, 2025
@cjyuan
Copy link
Contributor

cjyuan commented May 22, 2025

Changes look good. Well done!

@cjyuan cjyuan added Complete Volunteer to add when work is complete and all review comments have been addressed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. Reviewed Volunteer to add when completing a review with trainee action still to take. labels May 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Volunteer to add when work is complete and all review comments have been addressed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants