Bugfix: Correctly calculate Overlay position when inspecting a node within iframe(s) #552
Conversation
test/nested-frames/target.js
Outdated
| @@ -0,0 +1,59 @@ | |||
| /** | |||
There was a problem hiding this comment.
Can we add this example to the kitchensink instead?
There was a problem hiding this comment.
Sure... do you meantest/example/sink.js?
There was a problem hiding this comment.
It's easier to have them all in one place rather than create a new test for every case.
There was a problem hiding this comment.
Yup, that makes sense. I'll move it over there this evening
suchipi
force-pushed
the
bugfix_overlay_position_within_nested_iframes
branch
from
4b541be
to
2ff3105
Compare
|
Moved the example to sink and rebased on master. Not sure if the build output for sink should be committed, so I left that out. |
|
I did some testing, and it seems to work. Going to trust you on this one 😅 . |
|
Feel free to pull me back in if it breaks in the future 😉 Thanks! |
|
When can we expect to see this version released to the chrome store? I imagine a lot of people using react-storybook want this. |
|
Can you ping me about it again next week? |
|
Will do! |
|
@gaearon Pinging as promised! |
|
Thanks! I'm waiting for a few more changes in other PRs so I can all of this together. |
|
@gaearon Any updates? |
|
Not currently. I remember about releasing the DevTools. Right now it's secondary to our sprint to get React built with Rollup. This is a giant change internally, and we're very focused on it, but it won't take more than about a week more of my time. Then I'll be able to look at this again and cut a release. |
|
Okay. Thanks for the info! |
|
Looks like rollup work is done (facebook/react#9327); any chance this could be released? |
|
Rollup work also involves updating our internal FB scripts to build Rollup bundle, releasing an alpha for RN, and then updating React Native to remove internal dependencies and then use the flat bundle. I’m sorry it’s taking time, but I don’t want to leave it unfinished. |
|
Is there anything the community can do to help get this released sooner? As @rchanou indicated, this bugfix is significant for react storybook users (like myself). |
|
Would you like to help with reviewing and merging pending PRs that are (close to) approved? |
|
Sure; I should have some time to do that this Saturday. |
|
Thanks @gaearon. I will probably not have time to look at these until the upcoming weekend, but I took a look at a few other PRs in the meantime yesterday. |
|
Thanks! |
|
Sorry I didn't look at those PRs last weekend @gaearon. Hopefully I will have some time this weekend |
This fixes #443.
I didn't see any (automated) tests in the repo, so I have not added any. Should I have?
I created a test app with some nested iframes that can be used with the plain shell. To use it, build
test/nested-framesusing webpack and adddata-app-src="../../test/nested-frames/build/target.js"to the plain shell's target iframe.I have only tested this with the plain shell; I'm not sure how to test it as an extension.
I created some types for
DocumentandWindow. They only have the properties I cared about for this diff on them right now, but could be fleshed out eventually if desired. There might also be a better set of type definitions for the DOM API out there somewhere that could be used instead.