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

Don't try to access the devtools hook if we are cross-origin #3485

Merged
merged 2 commits into from
Apr 26, 2018

Conversation

tmeasday
Copy link
Member

This throws an exception and kills storybook in non-standard cases (like Chromatic's live mode). cc @ndelangen -- I think you saw this.

@Hypnosphi
Copy link
Member

We do a similar thing in Vue, it probably needs the check as well

@ndelangen
Copy link
Member

@Hypnosphi can you explain the chromatic diff? Seems like your handywork no? 😀

@Hypnosphi
Copy link
Member

Sure it is, but I have no idea why chromatic doesn't allow to approve it

@tmeasday
Copy link
Member Author

Yeah it looks like the master build/commit that this branched off (https://www.chromaticqa.com/build?appId=5a375b97f4b14f0020b0cda3&number=2309) included that change and was never approved.

The reason we can't approve the build for this PR comes down to the fact Chromatic's branch detection has been messed up by the change to TeamCity. Hopefully #3477 will resolve that.

I'll add the Vue check on this PR and rebase off current master, hopefully that'll sort out the chromatic thing.

This throws an exception and kills storybook in non-standard cases (like Chromatic's live mode). cc @ndelangen -- I think you saw this.
@codecov
Copy link

codecov bot commented Apr 26, 2018

Codecov Report

Merging #3485 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3485      +/-   ##
==========================================
- Coverage   37.41%   37.41%   -0.01%     
==========================================
  Files         455      455              
  Lines       10294    10297       +3     
  Branches      945      930      -15     
==========================================
+ Hits         3852     3853       +1     
- Misses       5861     5869       +8     
+ Partials      581      575       -6
Impacted Files Coverage Δ
app/react/src/server/config/globals.js 0% <0%> (ø) ⬆️
lib/core/src/server/build-dev.js 20% <0%> (-0.16%) ⬇️
addons/storyshots/src/vue/renderTree.js 60% <0%> (ø) ⬆️
lib/components/src/table/table.js 83.33% <0%> (ø) ⬆️
addons/info/src/components/markdown/code.js 76.92% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_routing.js 24.21% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 21.87% <0%> (ø) ⬆️
lib/ui/src/modules/ui/routes.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/api/index.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
... and 64 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 18f6521...279d4f9. Read the comment docs.

@tmeasday
Copy link
Member Author

Any idea what's happening with these Image stories in chromatic? This build is based on https://www.chromaticqa.com/build?appId=5a375b97f4b14f0020b0cda3&number=2341 where all stories were approved and does of course not change those stories

@tmeasday
Copy link
Member Author

Happy to merge this and fix the Chromatic problems separately of course.

tmeasday added a commit that referenced this pull request Apr 26, 2018
@tmeasday
Copy link
Member Author

Note I accidentally pushed 8984572 to release/3.4 to do the same thing there. I intended to make another PR.

I can't force push to remove it from history, I'm inclined to just leave it, LMK if that's bad @storybooks/team team

@Hypnosphi Hypnosphi added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Apr 26, 2018
@Hypnosphi Hypnosphi merged commit 559cc09 into master Apr 26, 2018
@Hypnosphi Hypnosphi deleted the tmeasday/check-react-access branch April 26, 2018 09:58
@Hypnosphi
Copy link
Member

Hypnosphi commented Apr 26, 2018

Any idea what's happening with these Image stories in chromatic?

Oh, that's probably because the images are random, we need to replace them with static placeholders

@tmeasday
Copy link
Member Author

Or we can just ignore then in Chromatic if it is too hard to do that

@tmeasday
Copy link
Member Author

But yeah, using https://placehold.it or the like is probably fine?

@Hypnosphi
Copy link
Member

Sure

@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug compatibility with other tools patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch react
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants