Skip to content
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

Ch/bg color change #456

Merged
merged 8 commits into from
Jan 3, 2018
Merged

Ch/bg color change #456

merged 8 commits into from
Jan 3, 2018

Conversation

ccchwang
Copy link

@ccchwang ccchwang commented Dec 18, 2017

This PR implements functionality to change background color on scroll. Each section has its own background color. This changes color for the body, nav, and footer.

Copy link
Contributor

@leobauza leobauza left a comment

Choose a reason for hiding this comment

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

👍 looks good, you have a failing test it looks like though.

@ccchwang
Copy link
Author

@leobauza, yeah I'll wait til @nhunzaker is back to merge!

<h3
className={'section__content__subheading -top' + browserClass}
>
<h3 className={'section__content__subheading -top' + browserClass}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a space here? Like will this turn into -top<browserClass>?

Copy link
Author

Choose a reason for hiding this comment

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

nope because there is a space built into browserClass but I can see why that's confusing so I will change it

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry :(. I must have missed it.

changeBgColor(oldSection, newSection) {
this.body.classList.remove(`bg-color-${oldSection}`)
this.body.classList.add(`bg-color-${newSection}`)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any concerns with the background color staying the section color when the user transitions to another page? Are there any other pages?

Copy link
Author

Choose a reason for hiding this comment

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

there are no other pages so should be okay! I'll keep an eye on that

@ccchwang
Copy link
Author

ccchwang commented Jan 3, 2018

@nhunzaker the CI audit test keeps failing for all my PRs even though I've run yarn format. Any ideas why?

@ccchwang
Copy link
Author

ccchwang commented Jan 3, 2018

@nhunzaker the CI audit test keeps failing for all my PRs even though I've run yarn format. Any idea why?

@codecov-io
Copy link

codecov-io commented Jan 3, 2018

Codecov Report

Merging #456 into ch/2-col-option will increase coverage by 2.83%.
The diff coverage is n/a.

Impacted file tree graph

@@                 Coverage Diff                 @@
##           ch/2-col-option     #456      +/-   ##
===================================================
+ Coverage            73.68%   76.51%   +2.83%     
===================================================
  Files                   44       44              
  Lines                 1368     1367       -1     
  Branches               255      253       -2     
===================================================
+ Hits                  1008     1046      +38     
+ Misses                 295      263      -32     
+ Partials                65       58       -7
Impacted Files Coverage Δ
packages/microcosm/src/addons/indexing.js 100% <0%> (+100%) ⬆️

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 1bc59c0...68e3886. Read the comment docs.

Chloe Hwang added 2 commits January 3, 2018 12:43
* starting styling layout and structure of mobile

* added styling for desktop layout

* tweaked heading margin

* ran yarn format

* ran yarn format

* removed prettier dev dependency

* Ch/qa scrolling (#458)
@ccchwang ccchwang merged commit 7440ffa into ch/2-col-option Jan 3, 2018
ccchwang pushed a commit that referenced this pull request Jan 3, 2018
* 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)
@ccchwang ccchwang deleted the ch/bg-color-change branch January 3, 2018 17:56
ccchwang pushed a commit that referenced this pull request Jan 3, 2018
* trial 1 for locked scroll behavior

* trying out two columns layout

* experimenting with react-intersection-observer

* set up IntersectionObserver and have it pull from json file

* restructured base layout for new design and did basic styling

* implementing intersection observer to change text on scroll

* added functionality to change view by button and started refactoring

* tweaked Intersection Observer to run smoother, added third section

* removed unnecessary comments

* remove comment

* ran yarn format

* fixed weird formatting by yarn

* fixed more weird formatting by yarn

* tweaking based on PR comments

* finished refactoring to change classnames on rerender

* refactored to rerender based on one state property only

* changed var name to be more clear

* got rid of unnecessary ids

* ran yarn format

* removed prettier as dev dependency and ran yarn format

* Ch/bg color change (#456)

* Ch/layout (#457)

* Ch/qa scrolling (#458)

* moved polyfill inside componentDidMount

* trying new way to import polyfill
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants