Skip to content

NW Class 5 - Khesiwe Dube- HTML/CSS - Week 1 Karma project #468

Open
KhesiweDube wants to merge 3 commits into
CodeYourFuture:masterfrom
KhesiweDube:master
Open

NW Class 5 - Khesiwe Dube- HTML/CSS - Week 1 Karma project #468
KhesiweDube wants to merge 3 commits into
CodeYourFuture:masterfrom
KhesiweDube:master

Conversation

@KhesiweDube
Copy link
Copy Markdown

@KhesiweDube KhesiweDube commented Jul 29, 2022

The title for your pull request should be made in this format

  • Your Name: Khesiwe Dube

  • Your City: NW5 Manchester

  • Your Slack Name: Khesiwe Dube

  • Module: 2

  • Week: 2

  • What did you find easy? creating nav tag and CSS

  • What did you find hard? some css sizing

  • What do you still not understand? some CSSand some tags

  • Any other notes?

@KhesiweDube KhesiweDube changed the title CITY CLASS_ NO -NW5 FIRST KHESIWE_NAME LAST_DUBE NAME HTML &CSS- MODULE - WEEK_NO 2 CITY CLASS_ NO -NW5 FIRST KHESIWE_NAME LAST_DUBE NAME KARMA CLONE PROJECT MODULE WEEK_NO 2 Jul 29, 2022
@KhesiweDube KhesiweDube changed the title CITY CLASS_ NO -NW5 FIRST KHESIWE_NAME LAST_DUBE NAME KARMA CLONE PROJECT MODULE WEEK_NO 2 CITY CLASS_ NO -NW5 FIRST KHESIWE_NAME LAST_DUBE NAME KARMA CLONE PROJECT MODULE WEEK 1 HTML &CSS Jul 29, 2022
@KhesiweDube KhesiweDube changed the title CITY CLASS_ NO -NW5 FIRST KHESIWE_NAME LAST_DUBE NAME KARMA CLONE PROJECT MODULE WEEK 1 HTML &CSS NW Class 5 - Khesiwe Dube- HTML/CSS - Week 1 Karma project Jul 29, 2022
@Ara225
Copy link
Copy Markdown

Ara225 commented Aug 24, 2022

Hi Khesiwe!
Overall, this looks like a really solid project some really good work here! The look and feel of the site is mostly spot on, especailly on the store page, and you've really tried to use sematitic html throughout.

Did notice a couple genral things as well as a few code specfic things (which I'll comment seprately below)

  • there aren't really any comments in the CSS or HTML- this is a bit of a pain as the CSS has to be quite complex (can't be helped CSS skews that way anyway) so it's a bit hard to get a sense of what it does
  • The main page is a bit off: Spacing of the three icons in the second second isn't quite right and the three social media icons are arranged vertically

Comment thread store.html
</ul>
</header>
<main class="store-main">
<fieldset>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

fieldset is normally used for grouping radio buttons to make clear they are related for accessability, rather than for the full form See https://syllabus.codeyourfuture.io/html-css/week-2/lesson#radio-input-fields https://developer.mozilla.org/en-US/docs/Web/HTML/Element/fieldset

Comment thread store.html
</div>
<div class="term">
By placing an order you agree to Karma's <a class="terms" href="https://example.com">Terms and Conditions</a>
<label for=""></label>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

You've used labels throughout which is brillant but the spec on the assignment does ask for validation on the form fields as well see https://syllabus.codeyourfuture.io/html-css/week-2/lesson#form-validation

Comment thread store.html
<div>
<button>Place my order</button>
</div>
</form>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Forgot to close the main tag :)

Comment thread css/style.css

}
h1 {
margin-top: 50px;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's best to use rem or em units when possible :)

Comment thread css/style.css
.store {
color: #DE6E49;
}
input[type="text"], input[type="number"], select { display: block; padding: 7px; border: 2px solid #efefef;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This is a clever CSS selector! Good work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants