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

Paper | Karla T #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Paper | Karla T #67

wants to merge 1 commit into from

Conversation

ktiktok96
Copy link

No description provided.

Copy link

@CheezItMan CheezItMan left a comment

Choose a reason for hiding this comment

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

Nice work Karla, the homepage looks pretty good, although the others are pretty basic. It's a good idea to feed your site into a validator just to find errors in the HTML/CSS.

<link href="https://fonts.googleapis.com/css2?family=Source+Code+Pro:ital,wght@1,300&family=Viaoda+Libre&display=swap" rel="stylesheet">

<!-- link to index-->
<href class="nav-link"><a href="index.html">Back to Main Page!</a></li>

Choose a reason for hiding this comment

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

href is not a tag. Did you mean to make it an li. Also this should be inside the body.

Suggested change
<href class="nav-link"><a href="index.html">Back to Main Page!</a></li>
<li ><a href="index.html">Back to Main Page!</a></li>

<link href="https://fonts.googleapis.com/css2?family=Source+Code+Pro:ital,wght@1,300&family=Viaoda+Libre&display=swap" rel="stylesheet">

<!-- link to index-->
<href class="navi-link"> <a href="index.html">My photos</a></li>

Choose a reason for hiding this comment

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

Similar to the above

<header >
<div class="logo">
<!-- <img src="img/About me Photo.JPG" alt=""> -->
<button class="nav-toggle" aria-label="toggle navigation">

Choose a reason for hiding this comment

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

You can't really make a ul a child of a button. Usually you just put text inside a button tag.

<li class="nav__item"><a href="#work" class="nav__link">My Work</a></li>
</ul>
</nav>
</div>

Choose a reason for hiding this comment

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

This div doesn't match.

<p>I am a student of computer science. Diving into this field with high spirits and full of life!</p>
</div>

<img src="img/Everyday_Shots_Buttercup_inSunset.JPG" class="about-me__img">

Choose a reason for hiding this comment

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

Images should have an alt attribute for accessibility and in case the image doesn't render.

.section__subtitle--about {
background: var(--clr-accent);
font-family: var(--ff-secondary);
margin-bottom: 1 em;

Choose a reason for hiding this comment

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

The margin can't be in em I think.

Suggested change
margin-bottom: 1 em;
margin-bottom: 5px;

z-index: 100;

transform: translateX(100%);
transition: transform 250ms cubic-bexier(.5, 0, .5, 1);

Choose a reason for hiding this comment

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

I'm not sure this is a transition value.

.portfolio {
display: grid;
grid-template-columns: repeat(auto-fit, minmax(300px, 1fr));
max-width: 0 auto;

Choose a reason for hiding this comment

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

You can't give 0 auto; as a max width.

text-decoration: none;
}

.footer__link:hoover,

Choose a reason for hiding this comment

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

Did you mean hover here?

opacity: .7;
}

.footer__link:hoover{

Choose a reason for hiding this comment

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

Did you mean hover here?

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