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

Ashley_Benson_SeaTurtles #98

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

Conversation

bensonaa1988
Copy link

Personal Portfolio Site

Congratulations! You're submitting your assignment!

Comprehension Questions

Question Answer
Did you have to resolve any issues when running the HTML Validator? If so, what were they?
Why is it important to consider and use semantic HTML?
How did you decide to structure your CSS?
What was the most challenging piece of this assignment?
Describe one area that you gained more clarity on when completing this assignment
Optional
Did you deploy to GitHub Pages? If so, what is the URL to your website?

Copy link

@anselrognlie anselrognlie left a comment

Choose a reason for hiding this comment

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

Looks good!

I liked seeing you try a few different styling approaches across your pages. Keep at it and see what else you can come up with! There were a few places where it looks like maybe you had started to add some tags for styling, but they didn't get used, and a minor validation issue, but nothing major.

Well done!

@@ -1,12 +1,29 @@
<!DOCTYPE html>

Choose a reason for hiding this comment

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

It looks like there are some images uploaded that weren't used. 😢

<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1.0">
<title>Document</title>
<link rel="stylesheet" href="/styles/index_styles.css">

Choose a reason for hiding this comment

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

Absolute paths can work well if your pages are in a variety of directories and you don't want to update copy/pasted code for styles/links. But we do need to make sure the root of our deployed server will match. If you try to deploy to github pages, you would want to follow the User/Org instructions here instead of the project site.

<header>
<h1> Ashley Benson</h1>
<nav>
<a href="http://127.0.0.1:5500/pages/about.html" id="about">About</a>

Choose a reason for hiding this comment

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

Since all the pages are in the same directory, we could link among them using relative paths

            <a href="about.html" id="about">About</a>

or if we used absolute paths, we should leave off the server info

            <a href="/pages/about.html" id="about">About</a>

<a href="http://127.0.0.1:5500/pages/portfolio.html" id="portfolio">Portfolio</a>

</nav>
<img src="/images/butterfly.jpeg" alt="header_img" id="header_img">

Choose a reason for hiding this comment

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

For some reason, it feels odd to me to have this image (which mostly fills the page and feels more like the main content) here in the header.

I'd probably put it together with the following p tag in a main content tag.


</div>
<div id="purple_2_left"></div>
<main>

Choose a reason for hiding this comment

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

This fails validation since main is not allowed to be a child of section.

<li>API's</li>
</ul>
</section>
<br>

Choose a reason for hiding this comment

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

Avoid using br solely for adding spacing, which can be accomplished instead with a margin-bottom.

Comment on lines +18 to +20
<li><a href="http://127.0.0.1:5500/pages/index.html">Home</a></li>
<li><a href="http://127.0.0.1:5500/pages/portfolio.html">Portfolio</a></li>
<li><a href="http://127.0.0.1:5500/pages/about.html">About</a></li>

Choose a reason for hiding this comment

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

                <li><a href="index.html">Home</a></li>
                <li><a href="portfolio.html">Portfolio</a></li>
                <li><a href="about.html">About</a></li>

Comment on lines +28 to +32
<ul class="tags">
<li>Python</li>
<li>TDD</li>

</ul>

Choose a reason for hiding this comment

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

Works great with the styling, and really clever use of a list to represent tags!

</section>
<section>
<h2>Hello Books</h2>
<p>My first API! Created a database to store book information. <a href="https://github.com/bensonaa1988/hello-books-api.git">Find the GitHub repo here</a>.</p>

Choose a reason for hiding this comment

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

With the lack of text-decoration on the anchor, it's hard to tell that there are links here. Users expect links to have underlines, and we shouldn't subvert this expectation without very good reason.

Comment on lines +71 to +93
.tags li {
color: whitesmoke;
border-radius: 10px;
padding: .5em;
font-size: .7em;
font-weight: bold;
}

.tags li:nth-of-type(1) {
background-color: royalblue;
}

.tags li:nth-of-type(2) {
background-color: orange;
}

.tags li:nth-of-type(3) {
background-color: mediumvioletred;
}

.tags li:nth-of-type(4) {
background-color: teal;
}

Choose a reason for hiding this comment

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

Consider moving the similar tag stuff from about into a shared css file that can be linked to from both the about and portfolio pages.

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