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

Add an embedded simulator to the documentation site #3335

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@jsierles
Contributor

jsierles commented Oct 11, 2015

Appetize.io graciously offered free simulator usage both for rnplay.org and the docs. This is a preview of how embedding the simulator might work: http://quick.as/X6qlfpYj2

The goal of this PR is to get feedback on the concept and design for We'd like to make it easy for others to add more examples like this one, so eventually everything on both iOS and Android have a runnable example.

Possible changes to UIExplorer are:

  • Enabling LinkingIOS deep linking (in progress, but only for iOS currently)
  • Fetching from iOS UserDefaults for deep linking (working, also could work on Android)

Website changes:

  • adding javascript for a modal, and the simulator frame embed
  • bundling UIExplorer's javascript, to be loaded remotely by the simulator
  • adding screenshots for each component
@jsierles

This comment has been minimized.

Show comment
Hide comment
@jsierles

jsierles Oct 11, 2015

Contributor

Will look at the test failures shortly, and squash commits once we settle on a final approach.

Also, we can add support for loading on a device soon, either via the Playground app, exponent, or a dedicated UIExplorer app that loads its jsbundle from the website. If the latter is useful, we can work on building and submitting them.

@vjeux @ide

Contributor

jsierles commented Oct 11, 2015

Will look at the test failures shortly, and squash commits once we settle on a final approach.

Also, we can add support for loading on a device soon, either via the Playground app, exponent, or a dedicated UIExplorer app that loads its jsbundle from the website. If the latter is useful, we can work on building and submitting them.

@vjeux @ide

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 11, 2015

Contributor

So exciting! I'll take a look at it tomorrow

Contributor

vjeux commented Oct 11, 2015

So exciting! I'll take a look at it tomorrow

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 11, 2015

Contributor
Contributor

vjeux commented Oct 11, 2015

@christopherdro

This comment has been minimized.

Show comment
Hide comment
@christopherdro

christopherdro Oct 12, 2015

Contributor

👍

Contributor

christopherdro commented Oct 12, 2015

👍

Show outdated Hide outdated Examples/UIExplorer/UIExplorerList.ios.js
}
componentWillMount() {

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

nit remove space

@vjeux

vjeux Oct 13, 2015

Contributor

nit remove space

Show outdated Hide outdated Examples/UIExplorer/UIExplorerList.ios.js
componentWillMount() {
LinkingIOS.addEventListener('url', function() {

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

nice :)

@vjeux

vjeux Oct 13, 2015

Contributor

nice :)

Show outdated Hide outdated Examples/UIExplorer/UIExplorerList.ios.js
componentWillMount() {
LinkingIOS.addEventListener('url', function() {
var url = e.url.replace('uiexplorer://', '').split('?');

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

it sucks that js doesn't come with a url parsing lib

@vjeux

vjeux Oct 13, 2015

Contributor

it sucks that js doesn't come with a url parsing lib

This comment has been minimized.

@jsierles

jsierles Oct 13, 2015

Contributor

We can use this or import qs.

@jsierles

jsierles Oct 13, 2015

Contributor

We can use this or import qs.

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

this is fine. Please don't import a giant library just for splitting a string

@vjeux

vjeux Oct 13, 2015

Contributor

this is fine. Please don't import a giant library just for splitting a string

Show outdated Hide outdated Examples/UIExplorer/UIExplorerListBase.js
render() {

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

remove

@vjeux

vjeux Oct 13, 2015

Contributor

remove

componentDidUpdate() {
if (this.props.route !== null) {
var route = decodeURI(this.props.route);
var component = this.props.components.concat(this.props.apis).find((component) => component.title === route);

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

what if component doesn't exist?

@vjeux

vjeux Oct 13, 2015

Contributor

what if component doesn't exist?

Show outdated Hide outdated website/layout/AutodocsLayout.js
</section>
</Site>
);
}
});
var EmbeddedSimulator = React.createClass({
render: function() {
if ( ! this.props.shouldRender ) return null;

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

please always use {} and put the return null; on its own line.

Also, no space inside of () and after !

@vjeux

vjeux Oct 13, 2015

Contributor

please always use {} and put the return null; on its own line.

Also, no space inside of () and after !

Show outdated Hide outdated website/layout/AutodocsLayout.js
<div className="modal-content">
<button className="modal-button-close">&times;</button>
<div className="center">
<iframe className="simulator" src={url} width="274px" height="550px" frameborder="0" scrolling="no"></iframe>

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

width and height are without px. I'm actually surprised it worked with it :p

@vjeux

vjeux Oct 13, 2015

Contributor

width and height are without px. I'm actually surprised it worked with it :p

Show outdated Hide outdated website/src/react-native/js/scripts.js
backdrop.classList.add('modal-open');
modal.classList.add('modal-open');

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

remove

@vjeux

vjeux Oct 13, 2015

Contributor

remove

Show outdated Hide outdated website/src/react-native/js/scripts.js
@@ -0,0 +1,29 @@
document.addEventListener("DOMContentLoaded", init);

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

nit: single quote

@vjeux

vjeux Oct 13, 2015

Contributor

nit: single quote

Show outdated Hide outdated website/src/react-native/js/scripts.js
}

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

nit: only one empty line between the two functions

@vjeux

vjeux Oct 13, 2015

Contributor

nit: only one empty line between the two functions

Show outdated Hide outdated Examples/UIExplorer/UIExplorer/RNUserDefaultsIOS.m
id response = [[NSUserDefaults standardUserDefaults] stringForKey:key];
if (response) {

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

rmeove empty line

@vjeux

vjeux Oct 13, 2015

Contributor

rmeove empty line

Show outdated Hide outdated Examples/UIExplorer/UIExplorer/RNUserDefaultsIOS.m
if (response) {
callback(@[[NSNull null], response]);
}

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

} else {

@vjeux

vjeux Oct 13, 2015

Contributor

} else {

Show outdated Hide outdated Examples/UIExplorer/UIExplorer/RNUserDefaultsIOS.m
callback(@[[NSNull null], response]);
}
else {

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

remove empty line

@vjeux

vjeux Oct 13, 2015

Contributor

remove empty line

Show outdated Hide outdated Examples/UIExplorer/UIExplorer/RNUserDefaultsIOS.m
RCT_EXPORT_MODULE()
RCT_EXPORT_METHOD(stringForKey:(NSString *)key callback:(RCTResponseSenderBlock)callback) {

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

remove empty line

@vjeux

vjeux Oct 13, 2015

Contributor

remove empty line

This comment has been minimized.

@vjeux

vjeux Oct 14, 2015

Contributor

?

@vjeux

vjeux Oct 14, 2015

Contributor

?

Show outdated Hide outdated Examples/UIExplorer/UIExplorer/AppDelegate.m
@@ -61,6 +61,8 @@ - (NSURL *)sourceURLForBridge:(__unused RCTBridge *)bridge
sourceURL = [NSURL URLWithString:@"http://localhost:8081/Examples/UIExplorer/UIExplorerApp.ios.bundle?platform=ios&dev=true"];
//sourceURL = [NSURL URLWithString:@"https://rnplay.org/uiexplorer.bundle"];

This comment has been minimized.

@vjeux

vjeux Oct 13, 2015

Contributor

remove?

@vjeux

vjeux Oct 13, 2015

Contributor

remove?

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 13, 2015

Contributor

All the comments are minor, the approach looks good.

Contributor

vjeux commented Oct 13, 2015

All the comments are minor, the approach looks good.

@jsierles

This comment has been minimized.

Show comment
Hide comment
@jsierles

jsierles Oct 13, 2015

Contributor

Added the suggested fixes and squashed commits. Tests should also pass!

Contributor

jsierles commented Oct 13, 2015

Added the suggested fixes and squashed commits. Tests should also pass!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 13, 2015

@jsierles updated the pull request.

facebook-github-bot commented Oct 13, 2015

@jsierles updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@jsierles updated the pull request.

facebook-github-bot commented Oct 14, 2015

@jsierles updated the pull request.

Show outdated Hide outdated website/src/react-native/js/scripts.js
document.addEventListener('DOMContentLoaded', init);
function init() {
if (!document) {

This comment has been minimized.

@vjeux

vjeux Oct 14, 2015

Contributor

should be typeof document === 'undefined'

@vjeux

vjeux Oct 14, 2015

Contributor

should be typeof document === 'undefined'

@vjeux

This comment has been minimized.

Show comment
Hide comment
@vjeux

vjeux Oct 14, 2015

Contributor

It seems like you haven't taken all my feedback into account. Are you planing to do more changes?

Contributor

vjeux commented Oct 14, 2015

It seems like you haven't taken all my feedback into account. Are you planing to do more changes?

@jozan

This comment has been minimized.

Show comment
Hide comment
@jozan

jozan Oct 14, 2015

Contributor

@vjeux We'll fix those shorly. Thanks for comments!

Contributor

jozan commented Oct 14, 2015

@vjeux We'll fix those shorly. Thanks for comments!

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@jsierles updated the pull request.

facebook-github-bot commented Oct 14, 2015

@jsierles updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@jsierles updated the pull request.

facebook-github-bot commented Oct 14, 2015

@jsierles updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@jsierles updated the pull request.

facebook-github-bot commented Oct 14, 2015

@jsierles updated the pull request.

Documentation: embed a runnable AlertIOS example using an appetize.io…
… simulator, along with supporting code for other modules
@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@jsierles updated the pull request.

facebook-github-bot commented Oct 14, 2015

@jsierles updated the pull request.

@facebook-github-bot

This comment has been minimized.

Show comment
Hide comment
@facebook-github-bot

facebook-github-bot Oct 14, 2015

@jsierles updated the pull request.

facebook-github-bot commented Oct 14, 2015

@jsierles updated the pull request.

@@ -22,7 +22,7 @@ server.noconvert = true;
// requests.
var queue = Promise.resolve();
glob('src/**/*.*', function(er, files) {
glob('src/**/*.*', { ignore: 'src/react-native/js/**/*' }, function(er, files) {

This comment has been minimized.

@vjeux

vjeux Oct 14, 2015

Contributor

sweet

@vjeux

vjeux Oct 14, 2015

Contributor

sweet

@jsierles

This comment has been minimized.

Show comment
Hide comment
@jsierles

jsierles Oct 14, 2015

Contributor

Closing in favor of #3419 and #3420.

Contributor

jsierles commented Oct 14, 2015

Closing in favor of #3419 and #3420.

@jsierles jsierles closed this Oct 14, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment