Skip to content

Commit

Permalink
Fixed issues mentioned in code review.
Browse files Browse the repository at this point in the history
  • Loading branch information
anderswift committed Aug 15, 2020
1 parent 1996da1 commit 7ecb9f5
Show file tree
Hide file tree
Showing 14 changed files with 114 additions and 67 deletions.
52 changes: 45 additions & 7 deletions blocks/cover/__link/cover__link.css
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
.cover__link {
display: block;
position: absolute;
background: url(../../../images/route66.png) 50% 50%;
background-size: cover;
min-height: 320px;
color: #fff;
text-decoration: none;
margin: 0;
Expand All @@ -9,7 +10,6 @@
justify-content: center;
align-items: center;
text-align: center;
position: absolute;
left: 0;
top: 0;
width: 100%;
Expand All @@ -18,7 +18,7 @@
}


.cover__link:before {
.cover__link:after {
content: "";
background: #2A2C2F;
opacity: .3;
Expand All @@ -27,10 +27,48 @@
left: 0;
width: 100%;
height: 100%;
z-index: -1;
z-index: 0;
transition: opacity .3s ease-in-out;
}

.cover__link:hover:before, .cover__link:focus:before {
.cover__link:hover:after, .cover__link:focus:after {
opacity: .8;
}
}


@media screen and (min-width: 525px) {

.cover__link {
min-height: 380px;
}

}


@media screen and (min-width: 768px) {

.cover__link {
margin-bottom: 88px;
min-height: 480px;
}

}

@media screen and (min-width: 900px) {

.cover__link {
min-height: 640px;
}

}



@media screen and (min-width: 1280px) {

.cover__link {
min-height: 740px;
}

}

1 change: 1 addition & 0 deletions blocks/cover/__subtitle/cover__subtitle.css
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
font-weight: 400;
margin: 0 16px;
max-width: 320px;
z-index: 1;
}

@media screen and (min-width: 768px) {
Expand Down
3 changes: 2 additions & 1 deletion blocks/cover/__title/cover__title.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,9 @@
font-size: 32px;
line-height: 35px;
font-weight: 900;
margin: 0 16px 16px 16px;
margin: 0 auto 16px;
max-width: 656px;
z-index: 1;
}

@media screen and (min-width: 768px) {
Expand Down
12 changes: 11 additions & 1 deletion blocks/photo-grid/__list/photo-grid__list.css
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
.photo-grid__list {
display: grid;
grid-template-columns: repeat( auto-fill, minmax(284px, 1fr) );
grid-template-columns: repeat( auto-fill, minmax(235px, 1fr) );
gap: 12px;
max-width: 352px;
margin: 0 auto;
}

@media screen and (min-width: 350px) {
Expand All @@ -12,9 +14,17 @@

}

@media screen and (min-width: 525px) {
.photo-grid__list {
max-width: 100%;
}
}


@media screen and (min-width: 769px) {

.photo-grid__list {
grid-template-columns: repeat( auto-fill, minmax(231px, 1fr) );
gap: 14px;
}

Expand Down
11 changes: 11 additions & 0 deletions blocks/place/__container/place__container.css
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
@media screen and (min-width: 600px) {

.place__container {
display: grid;
grid-template-columns: 35% auto; /* using percentage makes for smoother transition closer to brief between breakpoints */
gap: 32px 28px;
}

}


@media screen and (min-width: 768px) {

.place__container {
Expand Down
15 changes: 3 additions & 12 deletions blocks/place/__image/place__image.css
Original file line number Diff line number Diff line change
@@ -1,19 +1,10 @@
.place__image {
display: block;
width: calc(100% + 32px);
margin: 32px -16px;
width: 100%;
margin: 32px auto;
}

@media screen and (min-width: 461px) {

.place__image {
margin: 32px auto;
width: 100%;
}

}

@media screen and (min-width: 768px) {
@media screen and (min-width: 600px) {

.place__image {
margin: 0;
Expand Down
9 changes: 8 additions & 1 deletion blocks/place/__paragraph/place__paragraph.css
Original file line number Diff line number Diff line change
@@ -1,13 +1,20 @@
.place__paragraph {
font-size: 16px;
line-height: 26px;
margin: 0 0 24px 0;
margin: 0 16px 24px 16px;
}

.place__paragraph:last-child {
margin-bottom: 0;
}

@media screen and (min-width: 500px) {
.place__paragraph {
margin: 0 0 24px 0;
}
}


@media screen and (min-width: 984px) {

.place__paragraph {
Expand Down
15 changes: 10 additions & 5 deletions blocks/place/__title/place__title.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,25 @@
font-size: 32px;
line-height: 35px;
font-weight: 900;
margin: 0 0 32px 0;
margin: 0 16px 32px 16px;
}

@media screen and (min-width: 768px) {

@media screen and (min-width: 500px) {
.place__title {
font-size: 42px;
line-height: 46px;
margin: 0;
display: flex;
align-items: flex-end;
position: relative; /* small adjustment to better align with website text, as in mockup */
top: 6px;
}
}

@media screen and (min-width: 768px) {

.place__title {
font-size: 42px;
line-height: 46px;
}

}

Expand Down
2 changes: 1 addition & 1 deletion blocks/place/__website/place__website.css
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
line-height: 22px;
}

@media screen and (min-width: 768px) {
@media screen and (min-width: 600px) {

.place__website {
display: flex;
Expand Down
12 changes: 11 additions & 1 deletion blocks/place/place.css
Original file line number Diff line number Diff line change
Expand Up @@ -5,18 +5,27 @@
@import url(./__paragraph/place__paragraph.css);

.place {
margin-bottom: 48px;
margin: 0 auto 48px auto;
max-width: 460px;
}

.place:last-child {
margin-bottom: 0;
}


@media screen and (min-width: 600px) {
.place {
max-width: 100%;
padding: 0 16px;
}
}

@media screen and (min-width: 768px) {

.place {
margin-bottom: 72px;
padding: 0 24px;
}

}
Expand All @@ -25,6 +34,7 @@

.place {
margin-bottom: 80px;
padding: 0 48px;
}

}
3 changes: 3 additions & 0 deletions blocks/places/places.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
.places {
padding: 0 0 64px 0;
}
29 changes: 0 additions & 29 deletions blocks/root/root.css
Original file line number Diff line number Diff line change
@@ -1,20 +1,3 @@
/* Made some corrections, to follow process from this lesson about global styles:
* https://practicum.yandex.com/learn/web/courses/c4289437-1e2f-4625-a3c1-91b2306b8317/sprints/2372/topics/e7012cb1-1f16-407e-8b49-a39828bf2b31/lessons/be1df755-5efa-4eb9-8c7b-2c5eea66740d/
* I realize I had forgotten to include root class in index.html on body tag, but corrected is this okay to do?
* Is it better to include these global styles individually in index.css, or is either approach okay? I like the idea
* of organizing global styles separately under a root block, in case there are many, and to differentiate more easily what is
* unique to only one section and what is mixed into other blocks.
*/

@import url(../../vendor/fonts/fonts.css);

@import url(../content-container/content-container.css);

@import url(../link/link.css);
@import url(../list/list.css);
@import url(../dlist/dlist.css);


.root {
min-width: 280px;
font-family: 'Inter', Helvetica, Arial, sans-serif;
Expand All @@ -26,16 +9,4 @@
}


/* Because 'place' images are only 460px wide, I didn't want to stretch them larger past that width.
This led to a pretty ugly layout for 'places' section between 460px and 768px, so I added
this rule to correct that and stop page width from expanding until enough room for 2 columns in 'places'. */

@media screen and (max-width: 767px) {

.root {
max-width: 460px;
margin: 0 auto;
}

}

11 changes: 2 additions & 9 deletions index.html
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

<header class="header content-container">

<img class="logo logo_place_header" src="./images/logo.svg" alt="The USA logo">
<img class="logo" src="./images/logo.svg" alt="The USA logo">

<ul class="header__lang-links list">
<li><a class="header__lang-link header__lang-link_selected link" href="#">En</a></li>
Expand Down Expand Up @@ -213,14 +213,7 @@ <h2 class="place__title">Ohio</h2>
<a class="cover__link" href="https://www.nps.gov/nr/travel/route66/maps66.html" target="_blank">
<h2 class="cover__title">Historic Route 66</h2>
<p class="cover__subtitle">The Main Street of America from Santa Monica, Cal. to Chicago, Ill. within an iframe</p>
</a>

<!-- I was advised it is better to remove this image and use a background image, but I could not figure out for the life of me how
to get the relative scaling (width and height ratio of .cover section stays consistent at different screen sizes) without the img present.
I could only think of a way to do this with complicated relative padding and an extra div between .cover section and .cover__link. To me,
it seemed better to avoid the extra markup of a meaningless div and use the img tag. Any thoughts on how to do this better? -->
<img class="cover__image" src="./images/route66.png" alt="Photo of Route 66 highway.">

</a>
</section>


Expand Down
6 changes: 6 additions & 0 deletions pages/index.css
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
@import url(../vendor/normalize.css);
@import url(../vendor/fonts/fonts.css);

@import url(../blocks/root/root.css);
@import url(../blocks/content-container/content-container.css);
@import url(../blocks/link/link.css);
@import url(../blocks/list/list.css);
@import url(../blocks/dlist/dlist.css);

@import url(../blocks/header/header.css);
@import url(../blocks/logo/logo.css);
Expand All @@ -11,6 +16,7 @@

@import url(../blocks/photo-grid/photo-grid.css);

@import url(../blocks/places/places.css);
@import url(../blocks/place/place.css);

@import url(../blocks/cover/cover.css);
Expand Down

0 comments on commit 7ecb9f5

Please sign in to comment.