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

Static site - Jackie #42

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

Conversation

jackiewatanabe
Copy link

Static 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? Yes -- extra main tag...Also warnings that some sections I created don't have headings
Why is it important to consider and use semantic HTML? so it's easier for code to be read by others and also so digital readers can pick up more meaning for accessibility issues.
How did you decide to structure your CSS? From things that affected the most elements in all the files, to the hinge that might have the least impact. Also in order of top to bottom for elements in each html file
What was the most challenging piece of this assignment? Trying to come up with a doable vision of what I wanted my portfolio to look like
Describe one area that you gained more clarity on when completing this assignment how to better control where my elements will end up using the online block property

@kariabancroft
Copy link

Static Site

What We're Looking For

Feature Feedback
Baseline
Appropriate Git Usage Yes
Answered comprehension questions Yes - good thoughts all around
Page fully loads Yes
No broken links (regular or images) Yes
Includes at least 4 pages and styling Yes
HTML
Uses the high-level tags for organization: header, footer, main Yes
Appropriately using semantic tags: section, article, etc. Yes
All images include alternate text No - room for improvement here
CSS
Using class and ID names in style declarations Yes, though some are not used appropriately. In index.html for example, it doesn't make sense to create IDs for second-section, third-section etc. If you need to select in this way, it might make sense to add a single class and use the appropriate selector for the nth element.
Style declarations are DRY Not quite. I think this is a result of the item I noted above. There are a of duplicated styles that you could avoid if you used a class to generally cover the sections that are similar.
Overall Style looks really nice! I like the color scheme you used. Some more practice using classes rather than IDs for common styles would be good. One tip for linking to external sites (like your previous website) it would be good to research how to open the link in a new tab so your page doesn't get lost.

Copy link

@brookseakate brookseakate left a comment

Choose a reason for hiding this comment

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

I really like the overall styling, layout, and design of your site. The html is well-organized, clean, and makes really good use of semantics -- your <section> & <article> element nesting makes for really organized & parseable code. There are a bunch of really nice advanced touches, both with your styling/layout decisions and in your code style.

There are some opportunities to DRY out the CSS. After finalizing styles, you may want to review the CSS file looking for places with duplicate styling -- especially look for where you can share styles across pages using a common class, then add additional classes/IDs to a given element (or use pseudo-classes/pseudo-elements) to modify it further. You could also use a main css file to define styles that will be used across the site, then use individual css files per page for styles used only on that page -- and link both the main and individual stylesheet for a given page in its <head>. This can make the CSS easier to read and maintain.

Awesome site overall! Glad I got the opportunity to see it. 👍

<meta charset="utf-8">
<link rel="stylesheet" href="styles/main.css">
<link rel="stylesheet" href="font-awesome-4.7.0/css/font-awesome.min.css">
<link href="styles/normalize.css" rel="stylesheet">

Choose a reason for hiding this comment

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

It looks like this normalize stylesheet is overriding some of your styling (like cool fonts) -- is that intended?

If not (that is, if you want your styles to apply after the normalizing), you can move your main.css stylesheet below the normalize.css sheet in the <head> section and that should resolve.

<li><a href="portfolio.html">Code.Folio</a></li>
<li><a href="code-journal.html">Code.Journal</a></li>
<li><a href="http://jackiewatanabe.com">Past.life</a></li>
<li><a id="current-page" href="about.html">About.me</a></li>

Choose a reason for hiding this comment

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

Nice touch using styles to signify the current page in nav.

@@ -0,0 +1,416 @@
@import url('https://fonts.googleapis.com/css?family=Amatic+SC|Loved+by+the+King|Oswald|Lobster+Two|Quicksand|Roboto');

Choose a reason for hiding this comment

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

Looks like there are a couple fonts here that don't get used -- you could remove them when your design is finalized, to speed up page load time a bit.

}

header h2 {
font-family: 'Lobster Two'; cursive;

Choose a reason for hiding this comment

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

Typo in font list? Your other font lists use commas, but looks like this one got a semicolon, so the browser isn't picking up the fall-back option after the semicolon.


main {
min-height: 700px;
}

Choose a reason for hiding this comment

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

I think you have two blocks for a main selector -- could combine them.

border-color: black;
}

.page-posts article:last-of-type {

Choose a reason for hiding this comment

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

Nice use of :last-of-type

header {
background-color: #D0DD97;
position: fixed;
opacity: .5;

Choose a reason for hiding this comment

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

I love your header -- the opacity on fixed element is really cool!

header nav a {
text-decoration: none;
color: white;
text-transform: lowercase;

Choose a reason for hiding this comment

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

Your text-transforms are cool

padding-bottom: 0px;
margin-bottom: 0px;
}

Choose a reason for hiding this comment

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

Your #intro-text / #second-section / #third-section and #intro-title / #second-section-title / #third-section-title have a good number of property declarations in common. These could be a great place to use a shared class (ie .main-section / .main-title) to contain the shared properties, and include additional classes/IDs on any specific element for its additional styles.

@@ -0,0 +1,2337 @@
/*!

Choose a reason for hiding this comment

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

Ideally you'd only want to include files in your final product that are used by it, to reduce storage needs and/or increase load times. This can be tough when pulling in external libraries like FontAwesome, but you could remove extra jpgs that you didn't end up using.

One alternate option for FontAwesome would be to use a CDN (Content Delivery Network) for FontAwesome -- this would work more like Google Fonts, where you link to a URL instead of adding its files to your project -- the URL gives you access to any FontAwesome elements you might need. There can be a tradeoff with load speed, though, so it might not be the right choice for every case. Also you have to give them your email address, so 🤷‍♀️

@brookseakate brookseakate removed their assignment Jul 1, 2021
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