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

Yellow Box for react-dom #7360

Closed
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@keyanzhang
Member

keyanzhang commented Jul 27, 2016

This is a port of React Native's Yellow Box to react-dom. It's also similar to our internal ErrorMessageConsole module at Facebook.

It requires facebook/fbjs#165 which passes warning info from warning(...) to __showWarning(...) (the new global hook installed by react-dom).

A demo can be found here: https://jsfiddle.net/keyanzhang/mmupy3zh/15/embedded/result/. There's no fancy styling since we want to keep this component as simple as possible.

Thanks for reviewing! @spicyj

@zpao zpao added Component: DOM and removed Component: DOM labels Aug 3, 2016

@ghost ghost added the CLA Signed label Aug 3, 2016

@ghost ghost added the CLA Signed label Sep 27, 2016

@keyanzhang

This comment has been minimized.

Show comment
Hide comment
@keyanzhang
Member

keyanzhang commented Sep 27, 2016

@addyosmani

This comment has been minimized.

Show comment
Hide comment
@addyosmani

addyosmani Mar 6, 2017

Contributor

Beyond another rebase, is there further work to be done here before this lands? :)

Contributor

addyosmani commented Mar 6, 2017

Beyond another rebase, is there further work to be done here before this lands? :)

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Mar 6, 2017

Member

@addyosmani it depends on facebook/fbjs#165 (which itself requires some rebasing), and it will also need to be implemented in ReactDOMFiber.

It's not clear what the status is here since it's been sitting here for so long without review, maybe @spicyj or @gaearon can clarify whether this is something that we still want to do.

Member

aweary commented Mar 6, 2017

@addyosmani it depends on facebook/fbjs#165 (which itself requires some rebasing), and it will also need to be implemented in ReactDOMFiber.

It's not clear what the status is here since it's been sitting here for so long without review, maybe @spicyj or @gaearon can clarify whether this is something that we still want to do.

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Mar 6, 2017

Member

I believe this is on pause until we figure out a good public API for exposing hooks to warning and error reporting. @bvaughn built a version we use internally at Facebook that will inform what we do in open source.

Member

acdlite commented Mar 6, 2017

I believe this is on pause until we figure out a good public API for exposing hooks to warning and error reporting. @bvaughn built a version we use internally at Facebook that will inform what we do in open source.

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Mar 6, 2017

Contributor

There was also a related exploration on #8861 but as Andrew said, the team decided to put yellow box on hold for now in favor of higher-priority v16 stuff. We'll revisit it soon hopefully. 😄

Contributor

bvaughn commented Mar 6, 2017

There was also a related exploration on #8861 but as Andrew said, the team decided to put yellow box on hold for now in favor of higher-priority v16 stuff. We'll revisit it soon hopefully. 😄

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Mar 6, 2017

Member

Thanks for clarifying @acdlite @bvaughn! Should we close this then if it's not currently slated to be merged and isn't likely the implementation that will be used?

Member

aweary commented Mar 6, 2017

Thanks for clarifying @acdlite @bvaughn! Should we close this then if it's not currently slated to be merged and isn't likely the implementation that will be used?

@bvaughn

This comment has been minimized.

Show comment
Hide comment
@bvaughn

bvaughn Mar 6, 2017

Contributor

I think it's okay to close this PR for now. We may still end up using this as a starting point for whatever we do merge, but it's unlikely to be merged as-is.

Contributor

bvaughn commented Mar 6, 2017

I think it's okay to close this PR for now. We may still end up using this as a starting point for whatever we do merge, but it's unlikely to be merged as-is.

@aweary aweary closed this Mar 6, 2017

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Mar 6, 2017

Member

@addyosmani thanks for bringing this to our attention again!

Member

aweary commented Mar 6, 2017

@addyosmani thanks for bringing this to our attention again!

@bvaughn bvaughn referenced this pull request Aug 1, 2017

Closed

React 16 RC #10294

@gaearon gaearon referenced this pull request Oct 4, 2017

Closed

React 16 Spillover #11088

5 of 16 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment