New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Branch test, adding reviewer #1

Merged
merged 11 commits into from Jan 12, 2019

Conversation

Projects
None yet
2 participants
@tfolbrecht
Copy link
Owner

tfolbrecht commented Jan 11, 2019

馃憤

@tfolbrecht tfolbrecht requested a review from gsamaniego41 Jan 11, 2019

@gsamaniego41 gsamaniego41 merged commit 5054998 into master Jan 12, 2019

@gsamaniego41

This comment has been minimized.

Copy link
Collaborator

gsamaniego41 commented Jan 12, 2019

Congrats MVP complete!

@gsamaniego41
Copy link
Collaborator

gsamaniego41 left a comment

Good overall HTML markup. Solid Git skills. Good class names. MVP met! Spacing around the elements can be improved but that's something we can work on in the upcoming weeks.



<nav>
<img src="img/lambda-black.png" alt="logo">

This comment has been minimized.

@gsamaniego41

gsamaniego41 Jan 14, 2019

Collaborator

It wouldn't hurt to also link your logo back to the home page. It's good for UX.

<a href="index.html">Home</a>
<a href="#">About</a>
<a href="./index.html">Home</a>
<a href="./about.html"></a>

This comment has been minimized.

@gsamaniego41

gsamaniego41 Jan 14, 2019

Collaborator

Good job linking both header and footer <a>s to their respective pages. It looks like you're missing the text "About" on line 81.

width: 30%;
padding: 5px;
border-radius: 5px;
margin: 15px 0;

This comment has been minimized.

@gsamaniego41

gsamaniego41 Jan 14, 2019

Collaborator

Not part of the MVP but it's good UI/UX to add cursor: pointer to your buttons

margin: auto;
display: flex;
flex-wrap: wrap;
}

This comment has been minimized.

@gsamaniego41

gsamaniego41 Jan 14, 2019

Collaborator
  1. Perfect use case for flex-wrap: wrap, great job!
  2. Consider adding justify-content: space-between to create separation between your cards.
  3. Top and bottom margin can be added to create a space between your content.
<div class="box box7">Box 7</div>
<div class="box box8">Box 8</div>
<div class="box box9">Box 9</div>
<div class="box box10">Box 10</div>

This comment has been minimized.

@gsamaniego41

gsamaniego41 Jan 14, 2019

Collaborator

Nice job using multiple classes box1-box10 to preserve box's styling

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