Develop about jsx #50

rgoodie · 2018-02-09T13:39:07Z

Asking for a review. I moved the container class around in a few spots to allow for the full hero image. This changes the header to full screen, though content is still inside the container and constrained to decent dimensions. Please mostly see work in localhost:3000/about for changes.

After merging this back into develop I plan branching off it to build submit-a-sign that @nickttng has built in -prototype

codecov · 2018-02-09T13:42:26Z

Codecov Report

Merging #50 into develop will increase coverage by 6.32%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##           develop      #50      +/-   ##
===========================================
+ Coverage    89.89%   96.22%   +6.32%     
===========================================
  Files           21       23       +2     
  Lines           99      106       +7     
  Branches        10        7       -3     
===========================================
+ Hits            89      102      +13     
+ Misses           7        4       -3     
+ Partials         3        0       -3

Impacted Files	Coverage Δ
src/app.jsx	`100% <ø> (ø)`	⬆️
src/components/footer/about-us.jsx	`100% <ø> (ø)`	⬆️
src/components/faqs.jsx	`100% <ø> (ø)`
src/components/main.jsx	`100% <ø> (ø)`
src/components/footer/index.jsx	`100% <ø> (ø)`
src/components/footer/guidance.jsx	`100% <ø> (ø)`	⬆️
src/components/about/member.jsx	`100% <100%> (ø)`
src/components/about/index.jsx	`100% <100%> (ø)`
src/components/about/our-story.jsx	`100% <100%> (ø)`
src/components/about/team.jsx	`100% <100%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52639c3...c1d50c4. Read the comment docs.

kratsg

about.jsx should be loading from a team.json file that contains all of these things, and just loops over. There's a lot of copy/pasting that could be reduced.

styles-main.css is somewhat redundant, and should just be called main.css.

@nickttng will need to double-check the fonts, as I'm not the expert and can't comment on that.

kratsg · 2018-02-09T13:52:06Z

src/layouts/about.jsx

+        </div>
+      </div>
+
+      {/* ends conteiner */}


meltedspork · 2018-02-09T14:31:52Z

src/layouts/about.jsx

+        </p>
+      </section>
+      <div className="row py-5">
+        {/* <!--about-our-story--> */}


about-our-story can be broken in their own component (jsx)

meltedspork · 2018-02-09T14:32:38Z

src/layouts/about.jsx

+
+
+        {/* <!--about-our-name--> */}
+        <div className="col-md-6">


about-our-name also can be in their own JSX

meltedspork · 2018-02-09T14:34:12Z

src/layouts/about.jsx

+          <h3 className="tracked pb-2">Our Name</h3>
+          <p>
+            <strong>
+              You might be wondering, {'"'}What’s the story behind the name?{'"'}


its bit confusing reading this from code wise - can we simplify this something like

<strong> {`You might be wondering, "What’s the story behind the name?"`} </strong>

or

<strong> You might be wondering, {`"What’s the story behind the name?"`} </strong>

meltedspork · 2018-02-09T14:35:16Z

src/layouts/about.jsx

+        </div>
+      </header>
+
+      <div className="container">


This should be its own JSX and also it can be one components and we can use .map with it and are it reusable.

Separated out. Team is pulled from data.js file imported into a comp.

main.css renamed

meltedspork · 2018-02-09T14:36:27Z

I would like to see if we could import images and fonts with building rather than loading up as static - Webpack provides this. But that is best off as new issue.

meltedspork · 2018-02-10T03:26:44Z

public/data/people.json

@@ -0,0 +1,57 @@
+[{


can we rename to team.json ?

meltedspork · 2018-02-10T03:27:42Z

public/data/people.json

+        "full": "Jason Salerno",
+        "status": "DeafBlind",
+        "title": "Lead Product / Fullstack Development",
+        "img": "/images/team-jason.jpg"


dont think we need this img property - we can reuse by using id or first_name+ last_name.png

meltedspork · 2018-02-10T03:28:14Z

public/data/people.json

@@ -0,0 +1,57 @@
+[{
+        "full": "Jason Salerno",


can we do first_name and last_name ? and add id

meltedspork · 2018-02-10T03:29:52Z

public/data/people.json

+    },
+    {
+        "full": "Melissa Manak",
+        "status": "Deaf / Hard - of -Hearing",


Can we change this from status to identify_id and another to remapped to identity.json with identify id?

meltedspork · 2018-02-10T03:31:04Z

src/layouts/about/our-people.jsx

+import React from 'react';
+
+
+const OurPeople = () => (


can we rename this to Team ?

meltedspork · 2018-02-11T01:11:57Z

src/layouts/about/our-team.jsx

@@ -0,0 +1,17 @@
+// import React from 'react';


@rgoodie - take a look at gist i made - i have made adjust of how we can handle Team and Members components - https://gist.github.com/meltedspork/05350e1ec8cfda1b7ad16914ec63d103

As it follows React's Proptype - see: https://reactjs.org/docs/typechecking-with-proptypes.html
and also how we set state in constructor - see: https://reactjs.org/docs/state-and-lifecycle.html

Also for data, if we are doing static data -generally we should be using it as json format in flat file, not JS file to manage data and we can easily import json file and that would be bundled with transpiles.

meltedspork · 2018-02-11T01:16:41Z

src/layouts/about.jsx

+          </strong>
+        </p>
+        <p>
+          In fields where new technologies and technical terms are


same here with broken lines

kratsg · 2018-02-15T05:37:20Z

Last thing of note is about dealing with fonts. I'm not sure they're being imported correctly...

kratsg · 2018-02-15T05:40:05Z

Note: testing failed because of require.context which is not easily mockable in jest: facebook/create-react-app#517

meltedspork

NICE!!!

meltedspork · 2018-02-15T05:58:36Z

src/layouts/about/member.jsx

+
+const pathToMemberImage = require.context('../../images/team');
+
+export default class Member extends Component {


we can actually make this stateless function similar to submit-a-sign.jsx. but i am fine with either way. 👍

meltedspork · 2018-02-15T06:18:38Z

we probably can fix this require.context in test with https://github.com/webpack-contrib/mocha-loader instead.

- css added in - data/team.json along with images/team/*.png for people images using require.context for dynamic loading - evcohen/eslint-plugin-jsx-a11y#339 used for letting eslint know that react-router Link is a11y-compliant - images are converted to pngs, and resized down to 300pixels to save space where possible - added tests for team.json and mocked require.context

now using chai's expect

rgoodie requested review from kratsg, meltedspork and nickttng February 9, 2018 13:39

kratsg reviewed Feb 9, 2018

View reviewed changes

src/layouts/about.jsx Outdated

</div>

</div>

{/* ends conteiner */}

Copy link

Contributor

kratsg Feb 9, 2018

Choose a reason for hiding this comment

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

typo

meltedspork requested changes Feb 9, 2018

View reviewed changes

meltedspork assigned rgoodie Feb 9, 2018

meltedspork added frontend beta stage labels Feb 9, 2018

meltedspork reviewed Feb 10, 2018

View reviewed changes

meltedspork requested changes Feb 11, 2018

View reviewed changes

meltedspork force-pushed the develop branch from 45a9fa1 to 54bc673 Compare February 11, 2018 18:38

kratsg force-pushed the develop-about-jsx branch 3 times, most recently from 4d0a939 to 8f8340d Compare February 15, 2018 05:35

cd2bit deleted a comment from meltedspork Feb 15, 2018

cd2bit deleted a comment from rgoodie Feb 15, 2018

meltedspork approved these changes Feb 15, 2018

View reviewed changes

kratsg force-pushed the develop-about-jsx branch from 21f1549 to 44e5b13 Compare February 15, 2018 17:40

meltedspork added 2 commits February 15, 2018 23:27

refactored tests and minor in src

082ae87

now using chai's expect

remove unnecessary global

c1d50c4

kratsg approved these changes Feb 16, 2018

View reviewed changes

kratsg merged commit 3fbcb17 into develop Feb 16, 2018

kratsg deleted the develop-about-jsx branch February 16, 2018 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop about jsx #50

Develop about jsx #50

rgoodie commented Feb 9, 2018

codecov bot commented Feb 9, 2018 •

edited

Loading

kratsg left a comment

kratsg Feb 9, 2018

meltedspork Feb 9, 2018

rgoodie Feb 10, 2018

meltedspork Feb 9, 2018

rgoodie Feb 10, 2018

meltedspork Feb 9, 2018

meltedspork Feb 9, 2018

rgoodie Feb 10, 2018

rgoodie Feb 10, 2018

meltedspork commented Feb 9, 2018

meltedspork Feb 10, 2018

meltedspork Feb 10, 2018 •

edited

Loading

meltedspork Feb 10, 2018

meltedspork Feb 10, 2018

meltedspork Feb 10, 2018

meltedspork Feb 11, 2018

meltedspork Feb 11, 2018

kratsg commented Feb 15, 2018

kratsg commented Feb 15, 2018

meltedspork left a comment

meltedspork Feb 15, 2018

meltedspork commented Feb 15, 2018


		const pathToMemberImage = require.context('../../images/team');

		export default class Member extends Component {

Develop about jsx #50

Develop about jsx #50

Conversation

rgoodie commented Feb 9, 2018

codecov bot commented Feb 9, 2018 • edited Loading

Codecov Report

kratsg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltedspork commented Feb 9, 2018

Choose a reason for hiding this comment

meltedspork Feb 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kratsg commented Feb 15, 2018

kratsg commented Feb 15, 2018

meltedspork left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meltedspork commented Feb 15, 2018

codecov bot commented Feb 9, 2018 •

edited

Loading

meltedspork Feb 10, 2018 •

edited

Loading