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

Ebenezer #70

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

Ebenezer #70

wants to merge 18 commits into from

Conversation

edksam
Copy link

@edksam edksam commented May 23, 2020

Week 2 website.

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.

Well done, this looks a lot like the Karma website.

I've added some comments about when/where to use margin & padding. Also there's some parts of your css that could be improved in terms of organisation.

But there's some good use of flex to centre things, and it's good to see a media query. Your commit messages are also generally informative and clear about what you are changing, and why.


@media(max-width: 500px) {
Copy link

Choose a reason for hiding this comment

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

Good use of a media query here.

}
i.fa {
display: inline-block;
border-radius: 6rem;
Copy link

Choose a reason for hiding this comment

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

I think you want to be adding styling to the the li item rather than the icon itself.

This will ensure that the different widths of the icons doesn't affect the layout.

font-weight: 300;
text-align: center;
}
.social ul {
Copy link

Choose a reason for hiding this comment

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

Often the browser add extra margin and padding to a list. Here is where you'd want to remove it with padding: 0; & margin: 0;

font-weight: 500;
}
.card img {
width: 12.7rem;
Copy link

Choose a reason for hiding this comment

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

Instead of using an absolute value like rem. You could have used width: 33%; on the card (not the image), to keep all the cards the same width.

}
.logo {
width: 2.5rem;
margin-left: 15rem;
Copy link

Choose a reason for hiding this comment

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

Instead of margin left here, you could have added margin: 0 auto; to the container, or .navbar in this case.

color: #4c5058;
font-weight: 500;
}
.card img {
Copy link

Choose a reason for hiding this comment

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

There are some classes like this which could be organised a little better with comments so it's easier for the next person to work out which parts of the css refer to different parts of the page.

.hero-heading {
font-size: 4rem;
}
.hero-paragraph {
Copy link

Choose a reason for hiding this comment

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

Likewise here, it's a little bit difficult to track when there's hero styling earlier in your file, and some more styling here.

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