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

Joseph garcia stretch #2

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

Joseph garcia stretch #2

wants to merge 3 commits into from

Conversation

kingmoc
Copy link
Owner

@kingmoc kingmoc commented Apr 24, 2019

Here's some work on the @media queries stretch goals! Any feedback greatly appreciated good sir!

@kingmoc kingmoc requested a review from Ian84Be April 24, 2019 19:28
@@ -38,7 +39,7 @@ <h1>Innovation<br> On<br> Demand</h1>
</section>

<!-- Section Line -->
<div class = "line"> </div>
<div class = "line"> </div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

i would avoid using a div to create this effect. you could easily eliminate this "line" div and set a top-border on your "main-content" section. less code is always better.

@@ -112,7 +112,7 @@ section.services h1 {
/* Call To Action Selectors (index.html) */

section.cta {
background-image: url(C:/Users/kingmoc/Documents/git/User-Interface/great-idea-website/img/header-img.png);
background-image: url("../img/header-img.png");
Copy link
Collaborator

Choose a reason for hiding this comment

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

this path should probably be "./img/header-img.png" since it is going to be reading from where the index.html file is located

@@ -145,11 +145,12 @@ section.cta div h1 {
/* Main Content Selectors (index.html) */

section.main-content {
background-image: url("C:/Users/kingmoc/Documents/git/User-Interface/great-idea-website/img/mid-page-accent.jpg");
background-image: url("../img/mid-page-accent.jpg");
Copy link
Collaborator

Choose a reason for hiding this comment

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

again, this is a small error in this path that is preventing your images from being displayed

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is interesting in that when I tried running this from the live server - it wouldn't work using only 1 dot. Very interesting.

@@ -38,7 +39,7 @@ <h1>Innovation<br> On<br> Demand</h1>
</section>
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably no need to have a "cta" and "cta-text". one container should be enough for this element.

border: 1px solid black;
margin-left: 53px;
margin-top: 19px;
Copy link
Collaborator

@Ian84Be Ian84Be Apr 25, 2019

Choose a reason for hiding this comment

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

this margin-left and margin-top looks like a hacky way to center the button. this is where flexbox can save the day.

Copy link
Owner Author

Choose a reason for hiding this comment

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

You're exactly right! I'm getting better with flex in realizing the container - child relationship. Moving forward I shall use flex exclusively.


div.bottom-content {
margin-top: 235px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this margin-top would also work under LINE 161 without needing this extra selector

Copy link
Owner Author

Choose a reason for hiding this comment

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

Man - looking back at that I have no idea why I did that - smh!

}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

great job adding in these media queries. we will talk about this stuff at length next week in the responsive design modules.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think I added to many of them or something because I talked to another fellow in the program and he told me he only used one @media query ...

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