-
Notifications
You must be signed in to change notification settings - Fork 222
Issue #154 - Create Military Families Page #493
Conversation
@@ -56,7 +57,7 @@ class Home extends Component { | |||
} | |||
|
|||
setBgImage(location) { | |||
if (location.pathname === '/') { | |||
if (location.pathname === '/' || location.pathname === '/families') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be removed if we don't want the BgImage
on the families page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave it in here. I'll be fiddling with the Nav this weekend and this page isn't linked anywhere on the site.
While I'm in the Nav, I'll add a different image and try to set it up to dynamically choose a header image based upon route.
import React, { Component } from 'react'; | ||
import styles from './headerClipMaskLeft.css'; | ||
|
||
class HeaderClipMaskLeft extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component and the HeaderClipMaskRight
component will give us the clipping appearance we were seeing in the mockup. Not a big fan of this solution, but I think that if a browser doesn't support SVG, then it will simply not render it. Open to suggestions, but if it's too much of a hassle, I'll probably just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like, but I'd go a step further and join Left and Right to just make HeaderClipMask
for an import.
<FaQuoteLeft className={styles.quoteIcon} /> | ||
{this.state.quote.body} | ||
<FaQuoteRight className={styles.quoteIcon} /> | ||
“{this.state.quote.body}” |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mockup did not use Font Awesome quotes.
@@ -159,6 +160,10 @@ class Home extends Component { | |||
component={About} | |||
/> | |||
<Route | |||
path="/families" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this /about/families
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be it's own separate route because people will usually click it from the home page once we enable a link on the "Military Families and Spouses" card on the landing page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this is for thought. The definite changes I'd like to see:
-
Culmination of
<QuoteBanner>
and<JumboQuote>
into a reusable component. -
Culmination of Left and Right Clip Paths. Also, that breakpoint fix.
-
Decide on going bigger in global stylesheets or remaining the same. Regardless of the result, eliminate px sized fonts.
-
Left-align quote source.
-
Remove comments.
You matched the wireframe perfectly, but there's just some code smell to snub out.
text-align: 'center'; | ||
|
||
.factsHeading h2 { | ||
font-size: 50px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're doing our best to avoid px sizing fonts. You'll see on src/index.css
that we have a REM system that changes on break-points dynamically. I know that you we're aiming for the mock-up, and you matched it perfectly.
Try to balance what you've already got while choosing responsive text-sizes that make sense. This one in particular beats our h1 by 10 px on large screens - so it goes against our styles pretty heavily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the same token, perhaps we should up-size our text scale. Please feel free to start a debate on that in Slack. Sometimes I feel like the text on our site is too small.
If we're to scale our fonts up, we'll likely have some things break, but it'll be a good opportunity to wipe our project clean of all px sized fonts, and add a notch to the sustainable stylesheets belt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cleaned up the px
font sizes in goals.css
too.
@@ -28,10 +29,11 @@ class Facts extends Component { | |||
Only 19% of military spouses have adequate full-time employment. | |||
</li> | |||
</ul> | |||
<div className={styles.footnote}> | |||
<span>*Source </span><strong>pointsofflight.org</strong> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you align this to the left edge of the quotes above it?
position: relative; | ||
} | ||
|
||
@media screen and (max-width: 1700px) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you go with resizing the font's based upon our src/index.css
global styles, you will likely be able to get rid of this whole media query.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was able to get this effect by just removing this media query. So should be good.
src/scenes/home/families/families.js
Outdated
@@ -0,0 +1,39 @@ | |||
import React, { Component } from 'react'; | |||
// import family3 from '../../../images/Family-3.jpg'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove these comments?
import React, { Component } from 'react'; | ||
import styles from './headerClipMaskLeft.css'; | ||
|
||
class HeaderClipMaskLeft extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I actually like, but I'd go a step further and join Left and Right to just make HeaderClipMask
for an import.
@@ -0,0 +1,7 @@ | |||
.mask { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of making these twice, just pull from an above headerClipMask.css
since left and right styles are identical.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't do this exactly because they're not identical, but refactored the CSS to be more DRY.
@@ -56,7 +57,7 @@ class Home extends Component { | |||
} | |||
|
|||
setBgImage(location) { | |||
if (location.pathname === '/') { | |||
if (location.pathname === '/' || location.pathname === '/families') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave it in here. I'll be fiddling with the Nav this weekend and this page isn't linked anywhere on the site.
While I'm in the Nav, I'll add a different image and try to set it up to dynamically choose a header image based upon route.
.quotes { | ||
color: #445366; | ||
max-width: 1100px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between these styles and the styles I wrote for shared/quoteBanner
we've implemented styles for quotes twice. I think it may be useful to extract some mix of our styles into some kind of shared Quote component where we can pass props such as isHeader
and hasLines
.
You can see the <QuoteBanner>
component in action on /team
@@ -0,0 +1,7 @@ | |||
.mask { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems as though between 700px and 800px the clip path creates space between the browser frame edge and the image edge.
Check this out, btw: https://stackoverflow.com/questions/34354770/curved-line-at-the-bottom-of-border-in-css
@kylemh I'll be able to get to these changes sometime this weekend. Thanks for the review! |
@mwagz How are you doing with the changes? |
@Cooperbuilt I've had a really busy couple of weeks with work, but will be addressing these as soon as I'm able, which looks like sometime this week/weekend. |
This finalizes the Military Families and Spouses page as per Issue OperationCode#154. This also adds the requirements for the clipping appearance as was represented in the mockup.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One copy-pasta and a few more edits to the reusable component while you're in here!
Don't forget to link to the families page somewhere on landing - we want people to see your work!!
text-align: left; | ||
width: 75%; | ||
} | ||
|
||
.factsRight { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't want to give you more lame instructions on styles, so I made some final changes I'd like you to commit:
.factsRight {
background-position: 70% 30%;
border-radius: 40px;
flex: 0.48;
height: 50vh;
margin-top: 65px;
overflow: hidden;
}
.factItem {
padding-bottom: 24px;
}
/* most mobile browsers have annoying behavior with vh on scroll */
@media screen and (max-width: 420px) {
.factsRight {
height: 300px;
}
}
I changed background-position simply because it seems to work well on my iPhone, laptop, and desktop with that setting.
@@ -20,7 +20,7 @@ | |||
"react-error-overlay": "^1.0.7", | |||
"react-ga": "^2.2.0", | |||
"react-icons": "^2.2.5", | |||
"react-modal": "^1.7.7", | |||
"react-modal": "^2.3.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
@@ -5,7 +5,7 @@ import styles from './quoteBanner.css'; | |||
class QuoteBanner extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We lost the gray background that worked well on /team
and /history
. Can you add an optional boolean prop for isDark
. If isDark
, the font should be white and the quoteBanner background should be the former steely gray. Once you've done that, you can throw isDark: true,
onto line 89 of src/scenes/home/team/team.js
.
I'm going to close this due to inactivity. Feel free to open an updated PR with fixed conflicts and the requested changes! |
This finalizes the Military Families and Spouses page as per Issue #154. This also adds the requirements for the clipping appearance as was represented in the mockup. This is a request from @hollomancer posted in the OperationCode Projects channel in Slack.
Issue Resolved
Fixes #154