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/design tweak test #459

Closed
wants to merge 32 commits into from
Closed

Ch/design tweak test #459

wants to merge 32 commits into from

Conversation

ccchwang
Copy link

@ccchwang ccchwang commented Jan 3, 2018

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update
  • Refactor
  • Build tools
  • Documentation
  • Other, please describe:

Does this PR introduce a breaking change?

  • Yes
  • No

If yes, please describe the impact and migration path for existing applications:

Does this PR fulfill these requirements:

  • All tests are passing
  • yarn pretty has been run
  • Commits reference open issues

Additional Information

</aside>
)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you tracked the current section as component state? It could default to 0. Instead of manually assigning the class, you could make line 45 read like:

className={section === this.state.current ? '-active' : ''}

Then you avoid needing to select the elements on initial render too.

Copy link
Author

Choose a reason for hiding this comment

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

good call, I was able to use a state prop that already existed 👍🏼

@@ -57,9 +58,10 @@ export default class IndexPage extends React.Component {
onIntersection = observed => {
let entry = observed[0]
let section = parseInt(entry.target.dataset.section)
let isIntersecting = entry.intersectionRatio >= 1.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the 1.0 here the same threshold as observeOptions.threshold?

Copy link
Author

Choose a reason for hiding this comment

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

yes, I will save that in a var

@ccchwang ccchwang changed the base branch from ch/QA-scrolling to master January 3, 2018 18:59
@ccchwang ccchwang changed the base branch from master to ch/QA-scrolling January 3, 2018 19:00
@ccchwang ccchwang changed the base branch from ch/QA-scrolling to master January 3, 2018 19:01
@ccchwang ccchwang changed the base branch from master to ch/QA-scrolling January 3, 2018 19:03
@codecov-io
Copy link

codecov-io commented Jan 3, 2018

Codecov Report

Merging #459 into master will decrease coverage by 19.51%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #459       +/-   ##
===========================================
- Coverage   93.19%   73.68%   -19.52%     
===========================================
  Files          29       44       +15     
  Lines        1029     1368      +339     
  Branches      197      255       +58     
===========================================
+ Hits          959     1008       +49     
- Misses         59      295      +236     
- Partials       11       65       +54
Impacted Files Coverage Δ
packages/microcosm/src/utils.js 100% <ø> (+1.36%) ⬆️
packages/microcosm/src/action.js 100% <100%> (+3.41%) ⬆️
packages/microcosm/src/coroutine.js 100% <100%> (+56%) ⬆️
packages/microcosm/src/addons/action-form.js 97.61% <100%> (ø)
packages/microcosm/src/addons/indexing.js 0% <0%> (-100%) ⬇️
packages/microcosm/src/addons/action-queue.js 0% <0%> (-80%) ⬇️
packages/microcosm-preact/test/helpers.js
packages/microcosm-preact/test/setup.js
packages/microcosm-graphql/src/repo.js 0% <0%> (ø)
packages/microcosm-graphql/src/errors.js 0% <0%> (ø)
... and 23 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 25a2764...94e8983. Read the comment docs.

@ccchwang ccchwang changed the base branch from ch/QA-scrolling to 8-stable January 3, 2018 22:25
@ccchwang ccchwang changed the base branch from 8-stable to master January 3, 2018 22:26
@ccchwang ccchwang changed the base branch from master to ch/2-col-option January 3, 2018 23:03
@ccchwang ccchwang changed the base branch from ch/2-col-option to master January 3, 2018 23:03
@ccchwang ccchwang force-pushed the ch/design-tweak-test branch from f5c06dc to 94e8983 Compare January 4, 2018 17:07
@ccchwang ccchwang closed this Jan 4, 2018
This was referenced Jan 4, 2018
@ccchwang ccchwang deleted the ch/design-tweak-test branch January 5, 2018 17:12
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.

3 participants