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

Done is better than good. #108

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

Conversation

ArmchairGraduate
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.

This is a promising start.

Try to keep to the requirements of the project (especially required tag and style rule usage), and focus on the learning goals of the project. There will be other sites and other opportunities to dig into more complex layouts. At this point, we should be focused on gaining familiarity with semantic tags, and using selectors and rules to apply consistent styling across multiple pages.

For organization, we can determine which of our style rules are general site-wide rules, which are shared across several pages by function, and which are specific to certain pages, and put those rules together in distinct stylesheets to help us keep track of which rules are doing what.

But overall, this is a promising foray into web pages (and it's fun to scroll through the main page). 😀

@@ -0,0 +1,72 @@
<!-- <!DOCTYPE html>

Choose a reason for hiding this comment

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

Perhaps this page will get another shot at the big time in the future!

Comment on lines +6 to +10
@mixin align-center {
display: flex;
justify-content: center;
align-items: center;
}

Choose a reason for hiding this comment

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

This @mixin keyword is specific to a css preprocessor called scss (or sass, I think they both support it). So this wouldn't work with plain css. CSS is supposed to get a native version of mixins though I haven't been able to independently confirm this yet!

.intro_screen {
height: 100vh;
background-color: white; opacity: 0.5;
@include align-center;

Choose a reason for hiding this comment

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

The native CSS mixins is supposed to use the @apply directive rather than @include. Again, I haven't been able to confirm this is available yet.

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

Choose a reason for hiding this comment

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

Consider something a bit more descriptive of the page, or a bit more welcoming.


<nav>
<ul class="nav_links">
<li class="nav_link"><a href="/pages/about.html">Constuction</a></li>

Choose a reason for hiding this comment

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

In user-facing content, be careful of typos. They can slide by in code without too much issue (though we should be careful there as well), but a spell checking extension in VSCode (that's at least a little code aware so it doesn't freak out on programming terms) can be really helpful. I use Code Spell Checker.

* {
margin:0;
padding: 0;
}

Choose a reason for hiding this comment

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

consider also adding a box-sizing: border-box; rule to this as well. This can help us reason more easily about the sizes that content areas occupy. This isn't the default because the DOM originally used the content-box model, in which the width and height applied only to the content of a tag, and the border and padding were handled separately. It was always confusing, but since it was the default, we need to explicitly set our elements to use the more modern border-box;

padding: 0;
}

.centered {

Choose a reason for hiding this comment

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

Consider calling this something like .centered-container. .centered on its own sounds like something that we would want to apply to the content we want to center rather than the container it sits in.

.parallax_group {
position: relative;
height: 100vh;
transform-style: preserve-3d;

Choose a reason for hiding this comment

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

Not gonna lie, I have no idea what this does, even after reading the description. But this isn't unusual for CSS. There are many style rules that we tend to encounter through a tutorial or inspecting an example we like. It's a good idea to then pare down the sample to the minimum that we need to maintain the effect, and see what different values for the rules change. This can be a great way to build CSS skills if you find it interesting.

Comment on lines +79 to +84
/* #group_two {
z-index: 0;
} */
/* .group_two_z_index {
z-index: 0;
} */

Choose a reason for hiding this comment

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

Possibly related to the middle section not appearing? But the group 3 stuff is also commented out, but seems to work, so 🤷 . With all the perspective stuff and transforms, I'm not sure what is causing each section to appear vertically stacked. I tried commenting out the invisible middle section, and that caused the third group to get messed up, so there's definitely more research to do here!

.mid_layer_blurb {
font-size: 20px;
color: black;
width: calc(100% - 330px);

Choose a reason for hiding this comment

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

You had mentioned wanting this text to wrap for more narrow screens. One way to accomplish that is using @media queries to apply different rules at different screen sizes. Another thing could be to try adding a min-width to this element, since we saw that when it was too wide for the container, that forced a wrap.

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