Skip to content

London Class 9 - Karleen Richards - HTML/CSS Module Project - Week 1#505

Open
karleenmsrichards wants to merge 16 commits into
CodeYourFuture:masterfrom
karleenmsrichards:master
Open

London Class 9 - Karleen Richards - HTML/CSS Module Project - Week 1#505
karleenmsrichards wants to merge 16 commits into
CodeYourFuture:masterfrom
karleenmsrichards:master

Conversation

@karleenmsrichards
Copy link
Copy Markdown

Volunteers: Are you marking this coursework? You can find a guide on how to mark this coursework in HOW_TO_MARK.md in the root of this repository

Your Details

  • Your Name:
  • Your City:
  • Your Slack Name:

Homework Details

  • Module:
  • Week:

Notes

  • What did you find easy?

  • What did you find hard?

  • What do you still not understand?

  • Any other notes?

Copy link
Copy Markdown

@PeteLindsell PeteLindsell left a comment

Choose a reason for hiding this comment

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

This is looking really good!

I have made a few comments for possible improvements. Other than that some more info in the PR description would be useful but generally very good.

Comment thread css/style.css Outdated
Comment on lines +44 to +51
color: rgb(158, 156, 156);
text-decoration: none;
padding: 15px;

}

ul li a:hover {
color: #CE5C08;
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 is generally better to define colours consistently using HEX eg "#CE5C08" or rgb eg "rgb(158, 156, 156)"

Comment thread css/style.css Outdated
background-size: cover;
height: 500px;
font-size: 22px;
font-weight: 12;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

font-weights are usually "lighter", "normal", "bold" or in 100s eg "100", "200" etc

Comment thread css/style.css Outdated
margin-bottom: 0;
font-size: 40px;
font-weight: 10;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No need for an extra line here

Comment thread css/style.css Outdated
width: 120px;
height: 50px;
font-size: 14px;
border-radius: 4%;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I would usually go for a fixed px value for boarder radius, setting it as % makes it relative to the height and width of the box

Comment thread css/style.css Outdated
}

hr {
color: rgb(217, 219, 224);;
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 just want one ; here and on line 166

Comment thread index.html Outdated

<main>
<section class="section1">
<p class="section1-phrase1">Introducing Karma</p>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

A page should always have a h1 that describes the page uniquely in the site. This would be a good candidate.

Comment thread index.html Outdated
</section>

<section class="section2">
<p class="section2-phrase1">Everyone needs a little Karma.</p>
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 line looks like a title to this section so probably wants to be a h2

Comment thread index.html Outdated

<section class="section2">
<p class="section2-phrase1">Everyone needs a little Karma.</p>
<section class="section2-imgs-text">
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sections generally want titles so this is probably not suitable to be a section a list of items could be better eg

<ul>
   <li>  </li>
   <li>  </li>
</ul>

Comment thread index.html Outdated
</section>

</main>
<footer>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good use of footer here

Comment thread index.html Outdated
<p>Join us on</p>

<section>
<a href="#"><img class="footer-imgs" src="./img/twitter-icon.svg"></a>
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These images should have an alt attribute eg "Twitter"

karleenmsrichards and others added 13 commits October 27, 2022 23:15
1. Used fixed font sizes, eg rem throughout
2. Used fixed colours through out eg hex only for this page
3. Added an h1
4. Changed some sections tags to <div> as they did not fit the description of a section.
Added new division with img and text and added the css to style the new block.
Styled input fields and button.
Also added main image in HTML.
Customized Select tag
Fixed padding between labels and input
Changed the colour of the heading
1.Added customized checkbox tick and fixed margin between checkbox and label. Padding was also added to the tick when checked.
2.Adjusted size of radio buttons
1. Fixed gaps and padding between elements.
2. Changed CSS classes where appropriate.
Added a few media query parameters.
Adjusted CSS to for web page to fir screen size max width 576px.
Added media query at break point 576px for Hamburger menu and Nav list to replace original nav bar.
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