React Wrapper Step 1: Simple React Wrapper that never updates #7000
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,6 +17,7 @@ var api = require('./api'); | |
| var apiBlockly = require('./apiBlockly'); | ||
| var dontMarshalApi = require('./dontMarshalApi'); | ||
| var blocks = require('./blocks'); | ||
| var AppView = require('../templates/AppView.jsx'); | ||
| var page = require('../templates/page.html.ejs'); | ||
| var dom = require('../dom'); | ||
| var parseXmlElement = require('../xml').parseElement; | ||
|
|
@@ -646,29 +647,6 @@ Applab.init = function(config) { | |
| }); | ||
| } | ||
|
|
||
| config.loadAudio = function() { | ||
| studioApp.loadAudio(skin.failureSound, 'failure'); | ||
| }; | ||
|
|
@@ -761,90 +739,118 @@ Applab.init = function(config) { | |
| AppStorage.populateTable(level.dataTables, false); // overwrite = false | ||
| AppStorage.populateKeyValue(level.dataProperties, false); // overwrite = false | ||
|
|
||
| React.render(React.createElement(AppView, { | ||
|
There was a problem hiding this comment. I think this is a great way of transitioning from our current world to our new world. However, long term I think we want to discourage having this much content in our createElement call (to some extent referring to number of properties, but mostly referring to having full function definitions). If it's not difficult, I'd love to get to the place in the nearish future where we can have JSX instead of createElement which will discourage huge objects like this. In any case, no changes are needed in this regards for this PR There was a problem hiding this comment. I'd be happy to go through and extract the parameters into local variables, to make the expression of these props as compact as possible. That said, I think some of that will be mitigated as we get further into this process and break up page.html.ejs into smaller pieces that need fewer parameters. There was a problem hiding this comment. @Bjvanminnen would you like these better if the functions were simply defined outside the React.createElement call, then passed in by name? There was a problem hiding this comment. Yep, I would like that better. Though again, I think this is a fine first step (and results in a more minimal change with whitespace ignored). Moving out these functions might be a good step for a future PR. |
||
| renderCodeApp: function () { | ||
| return page({ | ||
| assetUrl: studioApp.assetUrl, | ||
| data: { | ||
| localeDirection: studioApp.localeDirection(), | ||
| visualization: require('./visualization.html.ejs')({ | ||
| appWidth: Applab.appWidth, | ||
| appHeight: Applab.footerlessAppHeight | ||
| }), | ||
| controls: firstControlsRow, | ||
| extraControlRows: extraControlRows, | ||
| blockUsed: undefined, | ||
| idealBlockNumber: undefined, | ||
| editCode: level.editCode, | ||
| blockCounterClass: 'block-counter-default', | ||
| pinWorkspaceToBottom: true, | ||
| // TODO (brent) - seems a little gross that we've made this part of a | ||
| // template shared across all apps | ||
| // disable designMode if we're readonly | ||
| hasDesignMode: !config.readonlyWorkspace, | ||
| readonlyWorkspace: config.readonlyWorkspace | ||
| } | ||
| }); | ||
| }.bind(this), | ||
| onMount: function () { | ||
| studioApp.init(config); | ||
|
|
||
| var viz = document.getElementById('visualization'); | ||
| var vizCol = document.getElementById('visualizationColumn'); | ||
|
|
||
| if (!config.noPadding) { | ||
| viz.className += " with_padding"; | ||
| vizCol.className += " with_padding"; | ||
| } | ||
|
|
||
| if (config.embed || config.hideSource) { | ||
| // no responsive styles active in embed or hideSource mode, so set sizes: | ||
| viz.style.width = Applab.appWidth + 'px'; | ||
| viz.style.height = (shouldRenderFooter() ? Applab.appHeight : Applab.footerlessAppHeight) + 'px'; | ||
| // Use offsetWidth of viz so we can include any possible border width: | ||
| vizCol.style.maxWidth = viz.offsetWidth + 'px'; | ||
| } | ||
|
|
||
| if (debuggerUi) { | ||
| debuggerUi.initializeAfterDomCreated({ | ||
| defaultStepSpeed: config.level.sliderSpeed | ||
| }); | ||
| } | ||
|
|
||
| window.addEventListener('resize', Applab.renderVisualizationOverlay); | ||
|
|
||
| var finishButton = document.getElementById('finishButton'); | ||
| if (finishButton) { | ||
| dom.addClickTouchEvent(finishButton, Applab.onPuzzleFinish); | ||
| } | ||
|
|
||
| var submitButton = document.getElementById('submitButton'); | ||
| if (submitButton) { | ||
| dom.addClickTouchEvent(submitButton, Applab.onPuzzleSubmit); | ||
| } | ||
|
|
||
| var unsubmitButton = document.getElementById('unsubmitButton'); | ||
| if (unsubmitButton) { | ||
| dom.addClickTouchEvent(unsubmitButton, Applab.onPuzzleUnsubmit); | ||
| } | ||
|
|
||
| if (level.editCode) { | ||
| // Prevent the backspace key from navigating back. Make sure it's still | ||
| // allowed on other elements. | ||
| // Based on http://stackoverflow.com/a/2768256/2506748 | ||
| $(document).on('keydown', function (event) { | ||
| var doPrevent = false; | ||
| if (event.keyCode !== KeyCodes.BACKSPACE) { | ||
| return; | ||
| } | ||
| var d = event.srcElement || event.target; | ||
| if ((d.tagName.toUpperCase() === 'INPUT' && ( | ||
| d.type.toUpperCase() === 'TEXT' || | ||
| d.type.toUpperCase() === 'PASSWORD' || | ||
| d.type.toUpperCase() === 'FILE' || | ||
| d.type.toUpperCase() === 'EMAIL' || | ||
| d.type.toUpperCase() === 'SEARCH' || | ||
| d.type.toUpperCase() === 'NUMBER' || | ||
| d.type.toUpperCase() === 'DATE' )) || | ||
| d.tagName.toUpperCase() === 'TEXTAREA') { | ||
| doPrevent = d.readOnly || d.disabled; | ||
| } | ||
| else { | ||
| doPrevent = !d.isContentEditable; | ||
| } | ||
|
|
||
| if (doPrevent) { | ||
| event.preventDefault(); | ||
| } | ||
| }); | ||
|
|
||
| designMode.addKeyboardHandlers(); | ||
|
|
||
| designMode.renderDesignWorkspace(); | ||
|
|
||
| designMode.configurePlaySpaceHeader(); | ||
|
|
||
| designMode.toggleDesignMode(Applab.startInDesignMode()); | ||
|
|
||
| designMode.configureDragAndDrop(); | ||
|
|
||
| var designModeViz = document.getElementById('designModeViz'); | ||
| designModeViz.addEventListener('click', designMode.onDesignModeVizClick); | ||
| } | ||
| }.bind(this) | ||
| }), document.getElementById(config.containerId)); | ||
| }; | ||
|
|
||
| /** | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it safe to silently ignore an undefined config.html?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, config.html comes from the server and can be empty. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we do our top-level rendering with React (prior to StudioApp.init) we want to skip this step, which would clobber everything React just rendered.
In my prototype I originally skipped this with a more explicit config variable:
That felt a little like duplicating state though - why even assign config.html if we're not going to use it, and then why introduce another config variable to say we're not using config.html when we can just check if it's empty?
Alternatively, I could tear out this line altogether if I also give NetSim a top-level React wrapper (it's the one remaining user of this old approach).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for explaining, it was not clear that this conditional is based on whether this app is using react to render. I think it would suffice to add a comment noting that
config.htmlwill be empty for apps which are rendering via React.