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

Omit rootcontainers from counting againt getAboveTheFoldCount #153

Closed

Conversation

lidawang
Copy link
Contributor

Should fix #144! Also added the test page I was using to make sure this worked.

<div>Three</div>
<RootElement when={this.data}><div>Four</div></RootElement>
</RootContainer>,
<div>Five</div>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to have some text in the content here that says what the viewer should expect to see if things are working properly.

@gigabo
Copy link
Contributor

gigabo commented May 13, 2016

Uh oh... looks like some of the automated tests are failing.

@lidawang - You can run those locally with npm test at the repo root.

@lidawang
Copy link
Contributor Author

Huhm I'm having some trouble with npm test - when I tried that I got errors, all originating from react-server's gulp test, something about eslint?

Errored while running npm script 'test' in 'react-server'
Error: Command failed: /bin/sh -c npm run test
/Users/lida.wang/code/react-server/packages/react-server/node_modules/eslint/lib/eslint.js:646
                    throw new Error("Definition for rule '" + key + "' was not found.");
                    ^

Error: Definition for rule 'keyword-spacing' was not found.
    at /Users/lida.wang/code/react-server/packages/react-server/node_modules/eslint/lib/eslint.js:646:27
    at Array.forEach (native)
    at EventEmitter.module.exports.api.verify (/Users/lida.wang/code/react-server/packages/react-server/node_modules/eslint/lib/eslint.js:619:16)
    at processText (/Users/lida.wang/code/react-server/packages/react-server/node_modules/eslint/lib/cli-engine.js:201:27)
    at CLIEngine.executeOnText (/Users/lida.wang/code/react-server/packages/react-server/node_modules/eslint/lib/cli-engine.js:361:26)
    at verify (/Users/lida.wang/code/react-server/packages/react-server/node_modules/gulp-eslint/index.js:19:17)
    at Transform._transform (/Users/lida.wang/code/react-server/packages/react-server/node_modules/gulp-eslint/index.js:37:18)
    at Transform._read (_stream_transform.js:166:10)
    at Transform._write (_stream_transform.js:154:12)
    at doWrite (_stream_writable.js:292:12)

It also repros when I try to run gulp test in /packages/react-server

@gigabo
Copy link
Contributor

gigabo commented May 13, 2016

@lidawang - Sounds like your gulp-eslint is out of date. Try an npm update in your repo root.

@gigabo
Copy link
Contributor

gigabo commented May 13, 2016

Oh, whoa, just read that again. Looks like it's picking up an eslint in your packages/react-server/node-modules.

Might be easiest to start fresh (in your repo root):

for d in `ls -d packages/*/node_modules`; do echo $d; rm -r $d; done
npm run bootstrap
npm test

@gigabo gigabo added the bug An issue with the system label May 13, 2016
@gigabo
Copy link
Contributor

gigabo commented May 16, 2016

@lidawang - Given the test weirdness here, I'm tempted to say let's just ditch this and wait for #161. 😖

That's going to obsolete it, anyway.

@lidawang lidawang closed this May 16, 2016
@lidawang lidawang deleted the lw-omit-rootcontainers-from-atfcount branch May 16, 2016 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An issue with the system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RootContainers count twice for getAboveTheFoldCount
2 participants