Skip to content
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

ITP | Sami Sardarzai | Module-User-Focused-Data | Week3 #302

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

Conversation

samisardarzai
Copy link

@samisardarzai samisardarzai commented Oct 31, 2024

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 COHORT_NAME | FIRST_NAME LAST_NAME | REPO_NAME | WEEK
  • I have tested my changes
  • My changes follow the style guide
  • My changes meet the requirements of this task

Changelist

Briefly explain your PR.
Added a new T-Shirt Order form with HTML5 form controls
Implemented input validation for customer name, email, color, size, and delivery date
Styled the form to enhance accessibility and user experience

Questions

Ask any questions you have for your reviewer.
Could you check if the date validation logic aligns with the requirements?
Any tips for simplifying my CSS for better readability?

Copy link

netlify bot commented Oct 31, 2024

Deploy Preview for cyf-ufd ready!

Name Link
🔨 Latest commit 936eb0c
🔍 Latest deploy log https://app.netlify.com/sites/cyf-ufd/deploys/672a08ad497d9c0008be07c8
😎 Deploy Preview https://deploy-preview-302--cyf-ufd.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@samisardarzai samisardarzai added the Needs Review Participant to add when requesting review label Oct 31, 2024
@cjyuan
Copy link

cjyuan commented Nov 2, 2024

  • What is the lighthouse score you got from Chrome? On mine it is not yet 100.

  • If you were asked to specify a constrained date range (says from 1/11/2024 to 28/11/2024) for the preferred delivery date, how would you enforce that with only HTML?

  • In terms of simplifying CSS, there doesn't seem much room left to simplify. You can ask ChatGPT and then see if you agree with the suggested change.

Form looks good to me. Personally I prefer the label to be left justified, but I am not qualified to give advice on design.

@cjyuan cjyuan added 👀 Review Requirements Changes requested to meet requirements Reviewed Volunteer to add when completing a review and removed Needs Review Participant to add when requesting review labels Nov 2, 2024
Copy link

@mrvicthor mrvicthor left a comment

Choose a reason for hiding this comment

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

Hi @samisardarzai ,
Excellent work here.
However, I have left a few comments here for you to address.

Choose a reason for hiding this comment

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

Hi @samisardarzai ,
Excellent work here with the form.

Choose a reason for hiding this comment

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

Hi @samisardarzai ,
Just a nit-pick, apart from the border, and box-shadow (where the size is fixed), I think it is better to use rem as opposed to px.

Copy link
Author

Choose a reason for hiding this comment

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

Hello @mrvicthor, thank you for your feedback! I will look into making the changes based on your suggestions as well.

@samisardarzai
Copy link
Author

Hello and thank you for your feedback @cjyuan

1. Lighthouse Score:
I ran the Lighthouse test on my setup, and both mobile and desktop scores are at 100/100. I’ve attached a screenshot to show the results.
Screenshot 2024-11-05 112312

2. Date Range Constraint:
To enforce a constrained date range in HTML, I set the min and max attributes on the date input field. This limits the date selection from 1st November 2024 to 28th November 2024, ensuring users can only pick a date within that range.

3. CSS Simplification:
After reviewing the CSS, I did simplify it further as much as possible. I combined some of the selectors and minimized the redundancy by grouping shared properties together. The changes mostly involved merging styles for input, select, and button elements, and using shorthand for the box-shadow property. This made the code cleaner without altering the design.

@samisardarzai samisardarzai added the Needs Review Participant to add when requesting review label Nov 5, 2024
@cjyuan
Copy link

cjyuan commented Nov 5, 2024

Form looks good!

@cjyuan cjyuan removed 👀 Review Requirements Changes requested to meet requirements Needs Review Participant to add when requesting review labels Nov 5, 2024
@samisardarzai
Copy link
Author

@cjyuan Thank you !

@samisardarzai samisardarzai added the Complete Participant to add when work is complete and review comments have been addressed label Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Complete Participant to add when work is complete and review comments have been addressed Reviewed Volunteer to add when completing a review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants