Skip to content
This repository has been archived by the owner on Oct 26, 2020. It is now read-only.

Week3 selina hussain #83

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

selinahussain
Copy link

No description provided.

…umbotron image. Used bootstrap to create navbar with toggle menu for mobile only. Css styling used to change background colour of navbar.
…d class to hide p text on mobile view. Added hover styling to nav links. Added styling to jumbotron.
…. added styling to card section with bg and button colours.
…ier to see and read. Compressed image files so it will take less time for the page to load.
Copy link

@pddys pddys left a comment

Choose a reason for hiding this comment

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

Looking good. The website is smart and so are the wireframes.

I've added some comments about minor fixes that can help you be more descriptive, or save time by not repeating the same classes twice.

Again the commit messages are descriptive which is good.

crossorigin="anonymous"
/>
<link
href="//maxcdn.bootstrapcdn.com/font-awesome/4.2.0/css/font-awesome.min.css"
Copy link

Choose a reason for hiding this comment

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

This link doesn't look correct, and it's not outputting Font Awesome into your file.

<!-- Jumbotron Section -->
<section class="container" id="about">
<div class="jumbotron text-center">
Copy link

Choose a reason for hiding this comment

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

Here, the container could have probably gone inside the jumbotron to give it a bit more width.

For example: https://www.w3schools.com/bootstrap4/tryit.asp?filename=trybs_jumbotron2&stacked=h

</div>
</div>
<div class="col-sm-12 col-md-6 col-lg-3">
Copy link

Choose a reason for hiding this comment

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

These cards may have benefitted from a bit of margin between each other at mobile widths.

Screenshot 2020-06-15 at 15 39 04

<div class="col">
<h2
class="display-12 font-weight-bold mt-5 pt-3 mb-5 pb-3 text-center"
Copy link

Choose a reason for hiding this comment

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

You can use my and py if the passing or margin is the same for top & bottom. That means you don't have to write out the classes twice.

https://getbootstrap.com/docs/4.4/utilities/spacing/

</div>
<div class="form-group row">
<label for="inputEmail3" class="col-sm-5 col-form-label"
Copy link

Choose a reason for hiding this comment

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

Good work on adding accessible labels for the forms.

opacity: 0.8;
}

.btns {
Copy link

Choose a reason for hiding this comment

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

btn-primary or btn-secondary might have been more appropriate classes in this case, as .btns isn't really descriptive to the next developer as to what the intention is.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants