Created 'Get involved' page -- need some help by ksmacleod99 · Pull Request #871 · OperationCode/operationcode_frontend

ksmacleod99 · 2018-02-18T01:53:09Z

Description of changes

Makes a Get Involved page

Issue Resolved

Satisfies Issue #12

Needs review; and I don't know how to begin with the styles for the isLeftFocused, isRighFocused, etc booleans. Needs path to volunteer page.
Also, some storybook info got sucked up into a commit, please advise on how to remove (or fix so it works properly :D )

also adds an image

Commits storybook info and I really hope it doesn't break anything

Hero banner component and Get Involved page

Styles header

it will never end

jjhampton · 2018-02-18T18:08:17Z

@ksmacleod99 re: 'some storybook info got sucked up into a commit, please advise on how to remove' - perhaps you ran one of the storybook NPM scripts while developing. I've noticed that this has a tendency to add (unwanted) changes that will be committed to Git.

The offending commit appears to be 73977ca . Since you've already submitted this PR and others may have pulled in this code to collab/review it, I would run git revert 73977ca. This will add a new commit that reverses the changes made in 73977ca. Then push the new (reverting) commit up to your remote branch.

The other option would be to use git's interactive rebase feature to delete commit 73977ca (which rewrites history), and then git push --force to update your remote branch with the new version of history. Only do that if you're sure that no one else has pulled in your earlier changes.

This reverts commit 73977ca.

ksmacleod99 · 2018-02-20T17:18:27Z

@jjhampton I think I did it... how bad did I break it?

jjhampton · 2018-02-25T22:26:15Z

@ksmacleod99 It looks like your revert commit got rid of some of the unwanted Storybook-related changes from commit 73977ca, but not all of them. The other files that have unwanted Storybook-related changes (and should have changes reversed by git revert) are:

.storybook/config.js
.storybook/webpack.config.js
stories/index.stories.js
yarn.lock

If you run git revert 73977ca again, I'd suggest inspecting the revert commit afterwards to ensure that the above files have the Storybook-related changes removed (by running git diff HEAD^, or possibly by using a Git GUI tool such as the GitHub desktop app or GitKraken). Once you are sure that those files have the Storybook-related changes removed, you can push again to your remote.

This reverts commit 73977ca.

ksmacleod99 · 2018-02-28T20:09:26Z

If the story book commits are gone, it should be good to go. I can work on isLeftFocus/isRightFocus on a different PR.

jjhampton · 2018-03-01T03:56:59Z

scripts/imageSize.js

+} else {
+  console.log('Test successful 💪 💪 💪 💪 💪 💪');
+  process.exit(0);
+}


Weird, this is the same file that's already been committed to master at 112fb6c.

lol what - how is this even possible

Because when I screw stuff up, I do it right. Go big or go home!

jjhampton · 2018-03-01T04:22:16Z

src/scenes/home/getInvolved/whyGive/whyGive.css

+  padding-left: 75px;
+  margin-top: 30px;
+  line-height: normal;
+}


These CSS selectors seem too specific for these li elements with class .check (used here and in the media queries above). There's really no need to use HTML tag + className in a chained selector unless you are overriding something else. Otherwise, it makes this component's CSS a bit more difficult to extend (by adding styles in the future).

CSS class name alone is sufficient here, the class check is not being used on any other elements within this component. You generally want to keep the specificity of your CSS selectors low, if possible.

https://css-tricks.com/strategies-keeping-css-specificity-low/

(edit) ^ also applies to the Volunteer and WhyGive component CSS styles.

Thanks! I'll fiddle with it.

jjhampton · 2018-03-01T04:27:41Z

src/scenes/home/getInvolved/successStories/successStories.css

+  flex-flow: row wrap;
+  align-items: center;
+  justify-content: space-around;
+}


This applies to all files in 'src/scenes/home/getInvolved/successStories':
This isn't very DRY - instead of copying over the entire SuccessStories component over to 'src'/scenes/home/getInvolved' from 'src/scenes/home/landing', if that component is going to be used on two scenes, it should probably be moved one level up to 'src/shared/components' so it can have a common reference point.

I think it was discussed that we might remove success stories from the home page to un-clutter it a little. But I will do whatever is decided upon.

jjhampton · 2018-03-01T04:36:03Z

src/scenes/home/getInvolved/check.svg

+		c-1.2,1.9-3.7,2.9-7.5,2.9c-3.9,0-6.1-0.2-6.8-0.5c-1.8-0.8-4-4.9-6.4-12.3c-2.8-8.2-4.2-13.3-4.2-15.4c0-2.2,1.9-4.4,5.6-6.5
+		C377.9,253.8,380,253.2,381.8,253.2"/>
+</g>
+</svg>


Are this file and 'src/images/icons/check.svg' both needed? I only see one reference to a check.svg added in this PR.

jjhampton · 2018-03-01T04:47:35Z

src/scenes/home/getInvolved/getInvolved.js

+        link="https://opencollective.com/operationcode#support"
+        isExternal
+      />
+      <LinkButton text="Volunteer" theme="blue" link="/signup" />


I don't think this LinkButton component needs to be nested here inside of the HeroBanner - I don't see any buttons anywhere?

Nuts - that's something I lost track of. It's on the wireframes to have a button in the hero banner. I'll remove it for now, and I can create an issue to try to allow link buttons as possible child components in the hero banner component.

jjhampton · 2018-03-01T04:53:10Z

src/scenes/home/getInvolved/getInvolved.css

@@ -0,0 +1,39 @@
+form {


Is there a reason for these styles targeting form elements? I don't see any forms in the markup, and also don't see .pageHeading used within this component. May not be needed.

We also cannot allow element level styles. If there is/are elements you need to apply styles to, inline them or make a class for them.

jjhampton · 2018-03-01T05:08:39Z

src/shared/components/heroBanner/heroBanner.js

+  subtitle: PropTypes.string,
+  isLeftFocus: PropTypes.bool,
+  isCenterFocus: PropTypes.bool,
+  isRightFocus: PropTypes.bool


@ksmacleod99 If you're wanting to hold off on using the properties isLeftFocus, isRightFocus, and isCenterFocus, best to just remove those properties here (they're not being used).

If you're trying to set different properties on something, you could have a corresponding CSS class for each of those boolean properties, inside ./heroBanner.css. Then, inside heroBanner.js you could use some JS logic to add the correct class to the JSX.

jjhampton

@ksmacleod99 - This looks really good - I really like all of the images chosen, the svg checkmarks, and the new HeroBanner component.

Besides the comments I added to the code, I noticed that some of the responsive layout is a bit off at when emulating smaller screen sizes (mainly iPad / tablet / larger mobile phones) on Chrome DevTools. A few instances of elements overlapping/off-center. I don't think that's a show-stopper, but if we merge this without fixing that, then we should probably create a separate issue for it.

Also - will we link to this page from anywhere in the main app (or externally) ?

kylemh · 2018-03-06T06:31:14Z

scripts/imageSize.js

+} else {
+  console.log('Test successful 💪 💪 💪 💪 💪 💪');
+  process.exit(0);
+}


lol what - how is this even possible

kylemh · 2018-03-06T06:32:34Z

src/images/icons/check.svg

+  <path d="m432.17 211.89c-45.767 0-83.03 37.263-83.03 83.03-1e-5 45.767 37.263 83.03 83.03 83.03 45.767 1e-5 83.03-37.263 83.03-83.03 0-19.786-6.6953-37.48-18.065-51.976l-7.8178 9.2659c9.9118 12.204 13.791 25.754 13.791 42.71 0 39.221-31.718 70.92-70.939 70.92s-70.939-31.699-70.939-70.92 31.718-70.939 70.939-70.939c12.428 0 21.724 1.7866 31.872 7.3911l7.4275-9.7539c-11.882-6.5621-24.797-9.7286-39.299-9.7286z" fill-rule="evenodd"/>
+  <path d="m401.44 284c3.3481 9e-5 5.8807 2.7473 7.5977 8.2416 3.4339 10.302 5.8807 15.453 7.3402 15.453 1.116 5e-5 2.275-0.85844 3.477-2.5755 24.124-38.632 46.445-69.882 66.963-93.748 5.3226-6.181 13.779-9.2716 25.369-9.2718 2.7471 1.7e-4 4.5928 0.25771 5.5373 0.77264 0.94418 0.51527 1.4164 1.1592 1.4165 1.9316-1.7e-4 1.2021-1.4167 3.563-4.2496 7.0826-33.138 39.835-63.873 81.901-92.203 126.2-1.9746 3.0906-6.0096 4.636-12.105 4.6359-6.1812 3e-5 -9.8299-0.25753-10.946-0.77267-2.919-1.2877-6.353-7.8552-10.302-19.703-4.4642-13.135-6.6964-21.377-6.6963-24.725-5e-5 -3.6056 3.0047-7.0826 9.0143-10.431 3.6915-2.0603 6.9538-3.0905 9.7869-3.0906"/>
+ </g>
+</svg>


I'm a big fan of SVGs, but we already have FA Icons involved as a dependency. See if you can leverage Font Awesome instead of this SVG (and the one you saved under /getInvolved

kylemh · 2018-03-06T06:33:26Z

src/scenes/home/getInvolved/getInvolved.css

@@ -0,0 +1,39 @@
+form {


We also cannot allow element level styles. If there is/are elements you need to apply styles to, inline them or make a class for them.

kylemh · 2018-03-06T06:33:36Z

src/scenes/home/getInvolved/getInvolved.css

+}
+
+@media (max-width:500px){
+  form {


kylemh · 2018-03-06T06:37:52Z

src/scenes/home/getInvolved/volunteer/volunteer.css

+  text-align: center;
+}
+
+ul {


Please no styles applied to elements. We don't have style splitting - any styles applied to elements get applied globally. It happens a few other times in the PR, so please just skim it for the culprits - I'll stop commenting on this issue going forward.

kylemh · 2018-03-06T06:57:48Z

src/scenes/home/getInvolved/successStories/successStories.js

+const SuccessStories = () => (
+  <Section title="Success Stories" theme="white">
+    <div className={styles.successStoriesContent}>
+      <ImageListItem image={image1} title={content.items[0].title} cardText={content.items[0].body} />


Once you've changed the content file to a .js file, you can use .map to spread the data across like so

kylemh · 2018-03-06T06:59:57Z

src/scenes/home/getInvolved/whyGive/whyGive.css

+  margin-top: 0;
+}
+
+.button {


This class shouldn't be necessary.

kylemh · 2018-03-06T07:01:11Z

src/scenes/home/home.js

            <Route path="/terms" component={Terms} />
            <Route path="/chapter_leader" component={ChapterLeader} />
            <Route path="/leadership_circle" component={LeadershipCircle} />
+            <Route path="/get-involved" component={GetInvolved} />


can you also add a Route for "/get_involved". I think you can use <Redirect>

@kylemh Is this due to usage of kebap-case vs snake_case? This won't be the only route that could use redirect to address that - there's several others in home.js already - we've had inconsistencies with route naming for quite some time.

Let's just create another issue to add redirects for all of those at once, so we can finish up this Get Involved page.

kylemh · 2018-03-06T07:02:14Z

src/shared/components/heroBanner/heroBanner.js

+import PropTypes from 'prop-types';
+import styles from './heroBanner.css';
+
+const propTypes = {


for the sake of intellisense in editors, I'd prefer these variables be named with a _ in front to discern between the prop-types API and the actual variable names.

Can you explain a little more? When I add an underscore before propTypes ES Lint throws an error, "Unexpected dangling '_' in '_propTypes' no-underscore-dangle"

Looks like we might want to turn off this ESLint rule: https://eslint.org/docs/rules/no-underscore-dangle

kylemh · 2018-03-06T07:02:50Z

stories/components/heroBanner/index.js

+
+storiesOf('HeroBanner', module).add('Default', () => (
+  <HeroBanner imageSrc={jamesBondJpg} title="Bond" subtitle="James Bond" />
+));


bless your soul for keeping Storybook up-to-date.

I'm trying. I have no idea what I'm doing and it's not working, but I'm trying lol

adds FA check, removed check.svg file

jjhampton · 2018-04-02T02:50:00Z

@ksmacleod99 This one's been open for quite some time now - how's it coming along? Do you need any help getting this finished up?

ksmacleod99 · 2018-04-02T21:49:35Z

I have everything except the redirect and the dangling underscore

This was removed in 27e9b64, but somehow got re-added. Not needed, as it stopped working.

Remove unused propTypes Assign propTypes object to class property instead of variable

Removed unneeded CSS styles Remove display: table and switch to flexbox Prevented header text from displaying over people in the hero background image

Added mobile-friendly styles Added whitespace btw/ checkmark and text

Redundant, already on landing page We can add other valuable content to this page later

jjhampton · 2018-04-14T19:56:51Z

@ksmacleod99 @wimo7083 @kylemh
I've pushed up some more changes and feel this is ready to review/merge. Page is accessible at
http://localhost:4000/get-involved if you pull down this PR branch.

Updates:

DRY'ed up the CSS
Used our reusable Section component within the sub-sections of Get Involved page
Cleaned up mobile responsiveness
Removed propTypes variables and unused proptypes
Set the HeroBanner height on desktop component to 480px - consistent w/ our already-in-use QuoteBanner and family banner on the About page
Removed the SuccessStories component from this page - as noted in Create Get Involved page #12 discussion, it's already on the landing page, which should have priority over this page.

After this is merged, we can work on the following. I'll create new issues as needed:

Continue work on the shared HeroBanner component to make it reusable for other pages / background images. The component doesn't yet support passing in props for left/right/center alignment yet, or props for a dynamic classname, but we can add that next and start using it elsewhere in the app. It works fine on the Get Involved page for now.
Figure out how we'll navigate/link to this page (from nav or elsewhere in app) - an issue needs to be created for that and several of our other new pages which have the same problem
Add redirects to home.js for multi-word-route names (as @kylemh suggested here).

Follow-on to 5784d04

Related to #931

ksmacleod99 added 9 commits January 26, 2018 11:54

Initial Commit

86833fa

Fixes styles

4dbb403

makes a file pretty

e74642c

Updating media queries

a8c1483

also adds an image

Storybook info

73977ca

Commits storybook info and I really hope it doesn't break anything

idk what or why this is happening

95a3559

progress so far

8607bfb

Hero banner component and Get Involved page

Styles Header

cf3ca4f

Styles header

more styling

ff84649

ksmacleod99 changed the title ~~Get involved~~ Created 'Get involved' page -- need some help Feb 18, 2018

updates styles

3cad47f

it will never end

Revert "Storybook info"

2cea7b2

This reverts commit 73977ca.

Revert "Storybook info"

9ea46bf

This reverts commit 73977ca.

jjhampton reviewed Mar 1, 2018

View reviewed changes

Remove static Storybook files

9ab45de

jjhampton reviewed Mar 1, 2018

View reviewed changes

jjhampton suggested changes Mar 1, 2018

View reviewed changes

kylemh suggested changes Mar 6, 2018

View reviewed changes

ksmacleod99 added 2 commits March 6, 2018 09:39

removed link button

685751e

FA check

05e904c

adds FA check, removed check.svg file

jjhampton mentioned this pull request Mar 6, 2018

Nav bar #854

Closed

updates FA icons

6418f1e

jjhampton added 5 commits April 14, 2018 12:02

Merge remote-tracking branch 'upstream/master' into getInvolved

ef7816a

Fix ESLint max-length errors

ffa9e62

Remove imageSize script again

86bcce0

This was removed in 27e9b64, but somehow got re-added. Not needed, as it stopped working.

Adjust heroBanner propTypes, per code-review requests

692c092

Remove unused propTypes Assign propTypes object to class property instead of variable

CSS cleanup

0a023da

Removed unneeded CSS styles Remove display: table and switch to flexbox Prevented header text from displaying over people in the hero background image

jjhampton self-assigned this Apr 14, 2018

jjhampton added 4 commits April 14, 2018 14:44

Clean up markup/CSS for whyGive, use Section component

4e87850

Added mobile-friendly styles Added whitespace btw/ checkmark and text

Adjust heroBanner height media queries for mobile

db00d66

DRY up shared CSS for GetInvolved sub sections

7e1af95

Remove SuccessStories from this page

5784d04

Redundant, already on landing page We can add other valuable content to this page later

jjhampton approved these changes Apr 14, 2018

View reviewed changes

jjhampton added 4 commits April 15, 2018 23:45

Remove SVG checkmark file

580b7cc

Remove duplicate SuccessStories files

fbccbc4

Follow-on to 5784d04

Clean up local names for image URLs

bc03553

Switch images to using S3 URLs

e1a5dde

Related to #931

kylemh approved these changes May 6, 2018

View reviewed changes

kylemh merged commit 82c7ce6 into OperationCode:master May 6, 2018

kylemh mentioned this pull request Jun 29, 2018

Create Get Involved page #12

Closed

Conversation

ksmacleod99 commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of changes

Issue Resolved

Uh oh!

jjhampton commented Feb 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksmacleod99 commented Feb 20, 2018

Uh oh!

jjhampton commented Feb 25, 2018

Uh oh!

ksmacleod99 commented Feb 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjhampton Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjhampton Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjhampton Mar 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjhampton left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjhampton commented Apr 2, 2018

ksmacleod99 commented Feb 18, 2018 •

edited

Loading

jjhampton commented Feb 18, 2018 •

edited

Loading

jjhampton Mar 1, 2018 •

edited

Loading

jjhampton Mar 1, 2018 •

edited

Loading

jjhampton Mar 1, 2018 •

edited

Loading

jjhampton left a comment •

edited

Loading

jjhampton commented Apr 14, 2018 •

edited

Loading