Skip to content

Conversation

@MohammedNaru
Copy link

@MohammedNaru MohammedNaru commented Oct 1, 2025

Pull Request Form Controls

Self checklist

  • I have titled my PR with Region | Cohort | FirstName LastName | 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 titled my PR with Region | Cohort | FirstName LastName | 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 created a webpage according to the Form control task

Questions

No questions.

@netlify
Copy link

netlify bot commented Oct 1, 2025

Deploy Preview for cyf-onboarding-module ready!

Name Link
🔨 Latest commit d39194b
🔍 Latest deploy log https://app.netlify.com/projects/cyf-onboarding-module/deploys/68e15223d5e08800091dd6d5
😎 Deploy Preview https://deploy-preview-879--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: 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.

@MohammedNaru MohammedNaru added the Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. label Oct 1, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Oct 1, 2025

  • Instead of adding your files in the top-level folder, you should modify Form-Controls/index.html (the file in Form-Controls folder).

  • You should not be using CSS in this exercise (as specified in Form-Controls/README.md). So you can delete the file 'Style.css` from your branch.

Please note that filenames are case sensitive. Index.html and index.html are treated as two different names.

Can you delete the two files you added, and update Form-Controls/index.html accordingly?

@github-actions
Copy link

github-actions bot commented Oct 1, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

@cjyuan
Copy link
Contributor

cjyuan commented Oct 1, 2025

The text in the PR description is in written in Markdown.
<!-- ... ---> is a HTML comment. If you are removing the comment from the template, you should delete the <!-- and -->.

I have restored part of the PR description from the PR template. Can you edit them and use Markdown to properly check the boxes?

Note:
You can use Markdown syntax, along with some additional HTML tags, to format your writing on GitHub,
in places like repository READMEs and comments on pull requests and issues.

To learn how to write in Markdown, embed image, tag users, and other advanced features,
visit GitHub Docs - Basic writing and formatting syntax.

@cjyuan
Copy link
Contributor

cjyuan commented Oct 1, 2025

After you have fixed your branch and PR description, please check out this general guide to see if you can further improve your work on your own, which in turn can help speed up the review process.

@MohammedNaru
Copy link
Author

I have now updated following the guidelines and removing CSS file

@cjyuan
Copy link
Contributor

cjyuan commented Oct 3, 2025

  1. If you clicked the "Files changed" tab in your PR (see pic), you can see all the modified files on this branch, but not all of them
    are part of this exercise. Can you keep this branch clean byremoving the files that are not part of this "Form-Controls" exercise?
image
  1. Your code is well-written, but it includes input elements that were not part of the given specification. Exploring different ideas is great, but when working with a specification, it’s important to stay within its requirements. Can you update your code to include what the spec asks for?

  2. The PR description is not yet properly written in Markdown syntax. Can you look up Markdown syntax for headers and checked checkboxes, and update the PR description accordingly? Here is the original content of the PR template in Markdown.

If formatted properly, the PR description should look something like this:
image

Note: I hid the description in the Changelist section.

@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 Oct 3, 2025
@github-actions
Copy link

github-actions bot commented Oct 3, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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

5 similar comments
@github-actions
Copy link

github-actions bot commented Oct 3, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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
Copy link

github-actions bot commented Oct 3, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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
Copy link

github-actions bot commented Oct 3, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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
Copy link

github-actions bot commented Oct 3, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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
Copy link

github-actions bot commented Oct 3, 2025

Your PR description contained template fields which weren't filled in.

Check you've ticked everything in the self checklist, and that any sections which prompt you to fill in an answer are either filled in or removed.

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

@MohammedNaru
Copy link
Author

I have made the necessary changes, please can you review

@MohammedNaru MohammedNaru 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 Oct 3, 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.

Good job in keeping the branch clean and the PR description properly formatted.

Just some minor changes needed.

  • The top-most header in the PR description is not quite formatted correctly in Markdown.
image

Comment on lines 21 to 23
<label for="password">Password:</label>
<input type="password" id="password" name="password" required minlength="6" placeholder="Min 6 characters">

Copy link
Contributor

Choose a reason for hiding this comment

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

Does the spec ask for password input?

Can you remove all input elements that are not specified in the spec (README.md) from this form?

Comment on lines 76 to 78
<footer>
<p>Form Controls Project – Onboarding Module</p>
</footer>
Copy link
Contributor

Choose a reason for hiding this comment

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

The original HTML code has a comment that specifies what the footer content should be. Can you update the footer content to meet that requirement?

@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 Oct 3, 2025
@MohammedNaru MohammedNaru added Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. and removed Needs Review Trainee to add when requesting review. PRs without this label will not be reviewed. labels Oct 3, 2025
@MohammedNaru
Copy link
Author

Good job in keeping the branch clean and the PR description properly formatted.

Just some minor changes needed.

  • The top-most header in the PR description is not quite formatted correctly in Markdown.
image

Update Please review again.

@MohammedNaru MohammedNaru 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 Oct 3, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Oct 3, 2025

There are still quite some input elements on the form that are not specified in the spec. Password was just one of them. Can you remove all input elements that are not specified in the spec?

@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 Oct 3, 2025
@MohammedNaru
Copy link
Author

There are still quite some input elements on the form that are not specified in the spec. Password was just one of them. Can you remove all input elements that are not specified in the spec?

Updated, please review

@MohammedNaru MohammedNaru 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 Oct 3, 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.

  1. It's a best practice to keep a branch clean so that reviewers can focus on related changes.
    Keeping unused code in comment can still make code review difficult. Can you keep your code as clean as possible?

  2. The spec has a "No CSS" requirement. The code in the style attributes are still CSS.
    You can keep the CSS code, but can you suggest a solution that does not involve using any CSS so that the page can still score 100 in Lighthouse accessibility?

Comment on lines 69 to 72
<label class="checkbox" style="display: block; margin-bottom: 16px; padding: 8px 0;">
<input type="checkbox" name="agree" required style="width: 24px; height: 24px; vertical-align: middle; margin-right: 8px;" />
Place Order
</label>
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the use of this checkbox?

@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 Oct 3, 2025
@MohammedNaru MohammedNaru 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 Oct 4, 2025
@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 Oct 4, 2025
@cjyuan
Copy link
Contributor

cjyuan commented Oct 4, 2025

Everything looks great now. A clean and professional code and PR. Well done!

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