Skip to content

London| Sep 25 ITP| Daniel Solomon | Sprint 2| Form-Controls #844

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 7 commits into
base: main
Choose a base branch
from
Open

London| Sep 25 ITP| Daniel Solomon | Sprint 2| Form-Controls #844

wants to merge 7 commits into from

Conversation

danisoloo
Copy link

@danisoloo danisoloo commented Sep 22, 2025 •
edited
Loading

London| Sep 25 ITP| Daniel Solomon |Sprint 2|Form-Controls

Self checklist

  • I have titled my PR with Region | Cohort | FirstName Last Name | Sprint | Assignment Title
  • My changes meet the requirements of the task
  • I have tested my changes
  • My changes follow the style guide

Changelist

I have replaced h2 by home work solution with my own name
I have edited and tested my code in order to meet the requirements of the task
I have improved accessibility to 100

@netlify Netlify
Copy link

netlify bot commented Sep 22, 2025 •
edited
Loading

✅ Deploy Preview for cyf-onboarding-module ready!

Name Link
🔨 Latest commit 23190bc
🔍 Latest deploy log https://app.netlify.com/projects/cyf-onboarding-module/deploys/68d98d5fe327010008293e9d
😎 Deploy Preview https://deploy-preview-844--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: 100 (no change 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.

@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

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).

3 similar comments
@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

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).

@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

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).

@github-actions GitHub Actions
Copy link

Your PR couldn't be matched to an assignment in this module.

Please check its title is in the correct format, and that you only have one PR per assignment.

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).

@danisoloo danisoloo changed the title London| Sep 25 ITP| Daniel Solomon | Sprint 1| Form-Controls London| Sep 25 ITP| Daniel Solomon | Sprint 2| Form-Controls Sep 22, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Sep 28, 2025

You forgot to add the "Needs review" label to this PR.

@danisoloo danisoloo added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Sep 28, 2025
@danisoloo
Copy link
Author

The label is done, I was struggling to add the label using my phone that is why. can you check it again and if there is something that I need to fix?

@danisoloo danisoloo added the 📅 Sprint 2 Assigned during Sprint 2 of this module label Sep 28, 2025
cjyuan
cjyuan reviewed Sep 28, 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. Currently a user can enter a name consisting of only space characters (e.g., " "). Can you enforce a stricter validation rule using the pattern attribute to disallow any name that contains only space characters?

  2. The Lighthouse score is not yet 100. Can you improve it to 100?

  3. Suggestion: You can try feeding your code to ChatGPT for second opinion, and see if it can give you some useful suggestions.

Form-Controls/index.html Outdated
Comment on lines 68 to 69
<!-- replace with your own name -->
<h2>By HOMEWORK SOLUTION</h2>
Copy link
Contributor

Choose a reason for hiding this comment

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

You missed updating this.

@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 Sep 28, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Sep 28, 2025

I just noticed the PR description does not have a "Changelist" section. Can you include a "Changelist" section and give a brief description of this PR?

@danisoloo danisoloo added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 28, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Sep 28, 2025

The code has been improved a lot. However, the following issues still remain:

  1. The Lighthouse Accessibility score is 95 but a score of 100 is needed to meet the requirement.

Since you are using only smartphone, you might not be able to use Lighthouse to test your page.

Here is a screenshot of what Lighthouse indicates, Touch targets do not have sufficient size or spacing.
image

Can you find out from ChatGPT "how to address the Touch targets do not have sufficient size or spacing issue without using CSS"?

  1. Currently a user can enter a name consisting of only space characters (e.g., " "). Can you enforce a stricter validation rule using the pattern attribute to disallow any name that contains only space characters?

You do not need to DM me on Slack. If you leave a comment on this PR, I will get notified by email.

@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 Sep 28, 2025
@danisoloo danisoloo added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Reviewed Volunteer to add when completing a review with trainee action still to take. labels Sep 28, 2025
@danisoloo
Copy link
Author

danisoloo commented Sep 28, 2025 via email

@cjyuan
Copy link
Contributor

cjyuan commented Sep 28, 2025 •
edited
Loading

The spec does not quite specify what constitutes a valid name, so any form of pattern is a good start. Good job.

The spec says "no CSS", and style="..." attribute is inline CSS. You don't have to change the code, but can you describe a way to resolve the Touch targets do not have sufficient size or spacing issue without using CSS?

I will mark this PR as "Complete" first. You can now submit Step 1 on the Course Portal and apply to become a trainee so that you can loan a laptop from CYF.

@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. labels Sep 28, 2025
@danisoloo
Copy link
Author

Thanks again for assessing my project. I’ll register as a trainee now.

@danisoloo danisoloo mentioned this pull request Sep 29, 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 all review comments have been addressed. 📅 Sprint 2 Assigned during Sprint 2 of this module
Projects
None yet
Milestone
No milestone
Development

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