Add support for multi windows. #278
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #278 +/- ##
=========================================
- Coverage 98.92% 97.62% -1.3%
=========================================
Files 3 3
Lines 278 337 +59
=========================================
+ Hits 275 329 +54
- Misses 3 8 +5
Continue to review full report at Codecov.
|
<body style={{color: "red"}}/>
add unit test.
There was a problem hiding this comment.
Thanks for taking the time to make a PR @vipcxj! We need a bit more time to look at this (there's quite a few changes) but I left a few comments in the mean time.
Also it looks like you have some merge conflicts with master, can you merge master into your branch and update the PR?
| @@ -78,6 +79,7 @@ | |||
| "phantomjs-prebuilt": "^2.1.14", | |||
| "react": "^15.x", | |||
| "react-dom": "^15.x", | |||
| "react-frame-component": "^1.0.3", | |||
There was a problem hiding this comment.
It looks like this is only being used in tests, can you move this to devDependencies?
package.json
Outdated
| @@ -31,6 +31,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "deep-equal": "^1.0.1", | |||
| "lodash": "^4.17.4", | |||
There was a problem hiding this comment.
We want to avoid taking lodash as a dependency. It looks like you can use ES6 functions like Array.isArray in the places you're using lodash. Is there an alternative _.groupBy and _.kebabCase you could use? If not can you add lodash.groupby and lodash.kebabcase as a dependency instead of the full lodash package?
test/HelmetDeclarativeTest.js
Outdated
| @@ -809,6 +811,32 @@ describe("Helmet - Declarative API", () => { | |||
| }); | |||
| }); | |||
|
|
|||
| it("use style object in body.", (done) => { | |||
| /* eslint-disable */ | |||
There was a problem hiding this comment.
Why do we need to disable every eslint rule for this block? Worst case scenario could you specify which rules need to be disabled? (similar to line 3 in this file)
# Conflicts: # src/HelmetUtils.js
|
I have merge master, no conflicts now. And I replace full version lodash with module version lodash. All test passed. |
|
Any updates on this one? We are currently having problems because Helmet doesn't support multiple windows. |
|
@smddzcy I can resolve conflicts when I am not busy. But I don't know how to pass codecov check. |
|
@vipcxj You can see the untested lines of code here: https://codecov.io/gh/nfl/react-helmet/pull/278/diff |
|
Wouldn't a kebob-case function be as simple as looking at charCodes for any character falling in the range 65 - 90 and replace it with "-" and that same charCode plus 32 ? `/**
|
For related react-helmet issue see nfl/react-helmet#278. Mostly this will be used to inject scripts/styles into the iframe.
Add props: window and document. With them, this library will work with multi windows environment.
For example, with https://github.com/ryanseddon/react-frame-component:
<Frame><Helmet window={this.context.window} document={this.context.document}><Script>...</Script></Helmet></Frame>The script will be add to the head of the iframe instead of the parent window.
update: add a feature.
<Helmet><body sytle={{color: 'red'}}/></Helmet>this works now.