Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bio page #8

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

Bio page #8

wants to merge 4 commits into from

Conversation

lukeware
Copy link
Collaborator

@lukeware lukeware commented Nov 4, 2016

Issue: #4

Hey all, it looks like the latest work by Kira in the bio page happened in the content branch which likely makes this pull request avoid. Doing it anyway, because, you know, homework! Plus clicking the green "create pull request" makes me feel powerful. Would should have happened here is I made the bio page and then created a new pull request that Kira could then work on. Since thats not how the assignments were design though, we didn't end up doing that until this week. That's not a problem and Shawn/Jackie know thats often what happens here. Go Sounders.

<div class="container">
<h1>Maya Angelou</h1>
<figure>
<img src="/media/maya-image.jpg" alt="" width="320" height="213">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Luke, I like your layout for the bio-page and the image you chose!

Might seem obvious but I'd rather over-communicate than miss it all together! When we merge we'll need to make sure that the media directory is included in the master so the audio and image files work correctly.
Since the featured-poem and content branches don't have a media folder yet, we should watch for this when we merge these branches to make sure we don't accidentally overwrite the media folder in the process.

Choose a reason for hiding this comment

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

Good catch, Kira. Definitely an important thing to keep our eyes open for!

</nav>

<!-- Main jumbotron for a primary marketing message or call to action -->
<div class="jumbotron">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice work! 👍 I like how you used the Jumbotron to style the bio-section prominently. Was it intentional to have the jumbotron outside of a container? If we switch the <div class="jumbotron"> and <div class="container"> tags, then clean up the div's at the end of the container, it fixes the footer alignment issue on this page.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, although you branched this bio.html page in the content branch and continued to develop from this code there, so we would probably just ignore this branch completely, right?


<p><a class="btn btn-primary btn-lg" href="#" role="button">Learn more &raquo;</a></p>
</div>
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Delete the extra <div> tag. See comment on line 64 above.

Copy link
Collaborator Author

@lukeware lukeware left a comment

Choose a reason for hiding this comment

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

This branch should be closed without being merged. All work here was copied to the bio.html page of the content branch. Code review the bio should be done there.

</nav>

<!-- Main jumbotron for a primary marketing message or call to action -->
<div class="jumbotron">
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, although you branched this bio.html page in the content branch and continued to develop from this code there, so we would probably just ignore this branch completely, right?

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.

3 participants