-
Notifications
You must be signed in to change notification settings - Fork 22
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
set up structure and JS #455
Conversation
Codecov Report
@@ Coverage Diff @@
## master #455 +/- ##
=========================================
+ Coverage 96.17% 96.3% +0.13%
=========================================
Files 30 29 -1
Lines 1071 1029 -42
Branches 200 197 -3
=========================================
- Hits 1030 991 -39
+ Misses 35 32 -3
Partials 6 6
Continue to review full report at Codecov.
|
@@ -0,0 +1,15 @@ | |||
import React from 'react' |
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.
These three graphic.js files (graphic1, graphic2, graphic3) correspond to the image in the left column. We will observe them for intersection
@@ -1,13 +0,0 @@ | |||
// antialised text for use on dark backgrounds |
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.
just some consolidating of scss files and general clean up here
this.changeText() | ||
this.changeButtonText(e.target) | ||
} | ||
|
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'm not sure if this file is the right place for all the functions above, wondering what is best practice for React/gatsby
|
||
changeSubheadingText(button) { | ||
this.subheadingTop.classList.toggle('-browserView', !this.onMicrocosmView) | ||
this.subheadingBottom.classList.toggle('-browserView', !this.onMicrocosmView) |
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 am changing some text via classes (e.g. "In Microcosm", "Meanwhile, in the browser...") but not sure if I should go the JS route because of potential accessibility issues
this.subheadingBottom.classList.toggle('-browserView', !this.onMicrocosmView) | ||
} | ||
|
||
switchView = e => { |
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 runs when you click the button under "Meanwhile, in the browser..."
} | ||
} | ||
|
||
|
||
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.
will take this entire thing out later, just using it for now
@@ -0,0 +1,15 @@ | |||
import React from 'react' | |||
|
|||
const Graphic1 = ({ fill }) => ( |
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.
Will fill
ever be used here?
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.
nope, good catch!
</figure> | ||
) | ||
|
||
export default Graphic3 |
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.
Are these graphics more or less the same conceptually? What you had a single Graphic
component that you passed different props into?
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.
yes, will most likely be the same so I will refactor that
</div> | ||
) | ||
} | ||
} |
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.
Why export two default namespaces? Is the second IndexPage overriding the first?
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 I deleted the second export!
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.
Oh jeez. Sorry, I should have checked the split view first!
</div> | ||
</div> | ||
</section> | ||
) |
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.
There is a lot of direct DOM manipulation in this component. If this component renders again, React might reverse changes you've made manually.
Instead of assigning state directly to this class, what if you used setState
? Then you could lean on React to do the DOM manipulation for you.
So instead of something like this.changeHeading()
, you'd just do this.setState({ heading: "..." })
. That would cause React to do a re-render for you, updating the heading on your behalf.
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.
The other benefit is that then all of your state for tracking what heading or graphic is displaying lives inside of the React component. You don't need to read from the DOM to get the current state of things. For me, this makes it much simpler to reason about rendering a page.
The render
method declares the ideal state. Once you have that set up, all you need to do is change a components state via this.setState
. Then React calls render again, and does all of the DOM manipulation to get your page to show the next set of content.
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.
yes, very good call!! I totally forgot about setState
so I will take advantage of that
> | ||
In | ||
</h3> | ||
<p className="section__content__text" id="text"> | ||
The <a href="TODO">Domains</a> are in charge of keeping state | ||
organized, and provide whatever data is necessary to the | ||
Presenter. A Presenter at it's core is a React Component, so 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.
its instead of it's?
map[graphic.dataset.section] = graphic | ||
return map | ||
}, {}) | ||
} |
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.
Could you derive this information from the index.json file instead of reading from the DOM?
export default class IndexPage extends React.Component { | ||
constructor(props) { | ||
super(props) | ||
} |
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 isn't necessary. By default, a class calls the constructor of its ancestor if it isn't specified.
@nhunzaker, made changes and it's ready for another pass through, thanks! |
@Chloehwang ignore the code coverage red flag. This is good to go! |
* started implementing functionality to change bg color * refactored footer for changing bg color, refactored scss to be more modular * rebased branch, refactored to take advantage of rerender * refactored color changing functionality so that nav and footer can change color as well * ran yarn format * removed prettier as dev dependency to use root rules instead * Ch/layout (#457) * Ch/qa scrolling (#458)
7440ffa
to
ade9de9
Compare
Started to lay foundation for marketing site and implement some core JS functionality.
This PR: