Skip to content
This repository was archived by the owner on Oct 2, 2024. It is now read-only.

London10-Afsha-Hossain-HTML/CSS-Week2 #242

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

Conversation

Afsha10
Copy link

@Afsha10 Afsha10 commented Mar 15, 2023

No description provided.

@Afsha10 Afsha10 changed the title Completed the form task London10-Afsha-Hossain-HTML/CSS-Week2 Mar 15, 2023
@Afsha10 Afsha10 added the review requested I would like a mentor to review my PR label Mar 21, 2023
@Afsha10 Afsha10 requested a review from SallyMcGrath March 21, 2023 22:09
@SallyMcGrath SallyMcGrath removed the review requested I would like a mentor to review my PR label Mar 23, 2023
Copy link
Member

@SallyMcGrath SallyMcGrath left a comment

Choose a reason for hiding this comment

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

Thanks for this, @Afsha10 ! Your code is valid and scores 100 on Lighthouse. Well done!

I have dropped a few comments and asked a few questions. Take a look and have a think. Few more questions:

  • There are a few requirements you have missed. Can you spot them?
  • Would you be happy to put this design on your portfolio? If not, why not, and what would you do to make it a portfolio piece?

@@ -0,0 +1,3 @@
{
"liveServer.settings.port": 5501
Copy link
Member

Choose a reason for hiding this comment

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

You've added a bunch of files from your repo that shouldn't be here. Usually this happens when you do git add . . This is not a good way to add your files with git. Add your files one by one, individually and with purpose.

Later in the course, you might have 5,000 files or more (node modules) that should not be committed. Imagine the mess!

Copy link
Member

Choose a reason for hiding this comment

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

Remove this file please.

margin: 1rem;
}

.select-delivery-date {
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you've applied the same styles to all these classes. Is there a way you could apply one style to all the classes? What does "class" actually mean?

<section class="select-colour">
<label for="color-picker">Choose your t-shirt colour from the three available colours</label>
<input type="color" list="presets" id="color-picker">
<datalist id="presets">
Copy link
Member

Choose a reason for hiding this comment

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

Love a datalist!

@@ -16,6 +16,59 @@ <h1>Product Pick</h1>
<form>
<!-- write your html here-->
<!-- try writing out the requirements first-->
<fieldset class="validate-account">
Copy link
Member

Choose a reason for hiding this comment

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

Nice use of fieldset.

</datalist>
</section>

<section class="select-size">
Copy link
Member

Choose a reason for hiding this comment

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

You've used section here and not fieldset. Can you talk through why?

</section>

<section class="select-size">
<div>
Copy link
Member

Choose a reason for hiding this comment

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

And here you've used a bunch of divs. What are these divs doing in the code? What effect do they have?

Something special happens with fieldset and radio inputs. Can you find out what?

@SallyMcGrath SallyMcGrath added the reviewed A mentor has reviewed this code label Mar 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
reviewed A mentor has reviewed this code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants