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

React Wrapper Step 3: Split page.html.ejs along column divide #7051

Merged
merged 7 commits into from Mar 1, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Feb 29, 2016

Splits the page.html.ejs template into two files, one for the contents of #visualizationColumn and one for the contents of #codeWorkspace. Passes a separate render function for each section to the top-level React component.

This has several side effects:

  • I've separated out the properties passed to visualizationColumn.ejs vs codeWorkspace.ejs so they only receive what they need. This had to be done on an app-by-app basis since they each render a little differently.
    • In most cases there's a nice clean separation, which will help with further refactoring down the road.
    • applab and gamelab have the worst entanglement / props needed in both views. I think the debugger might be to blame.
  • We break out our first unique top-level component NetSimView because NetSim does not have a #visualizationColumn or a #codeWorkspace so it can't participate in this transform.
  • Some common boilerplate between AppView and NetSimView gets factored into StudioAppWrapper, including the rule for whether to render RotateContainer (see related conversation on #7026).
  • ProtectedStatefulDiv now takes an id property (among others), since we have lots of code that depends on access to #visualizationColumn and #codeWorkspace.
  • #visualizationResizeBar is now rendered entirely by React, though it's also locked and will not update since we manually bind handlers against it outside of React.

Step 3 from this design document.

Testing

Worst bug surface in this change is probably from typos in breaking out the properties passed to the EJS rendering functions.

  • Run UI tests locally via chromedriver
  • Run UI tests via Saucelabs against local
  • Run eyes tests via Saucelabs against local
  • Manually verify each app type in allthethings

Progress

Pretty cool to see React slowly reach into the page:
screenshot from 2016-02-29 15-51-01

islemaster added 6 commits Feb 29, 2016
Now in two parts:
- visualizationColumn.html.ejs
- codeWorkspace.html.ejs

Each app has its own version of rendering these two templates,
hence we need to touch each app's init() function again.

Once again this is more verbose for now, but the separation of
concerns should lead us toward less duplication in the end.

Ran UI tests locally.
Grabs common code from NetSimView and AppView and creates
a component they both use, while letting them stay
as separate top-level components.
Changes ProtectedStatefulDiv to no longer require a renderContents function.
It also now passes _all_ of its props (via JSX spread attributes) to
its root DOMComponent.
Together, this makes it easy to define an empty div with an id and
some classes or style attributes, that should never be updated by
React - like our resize bar, for example.
@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Mar 1, 2016

<span><%= msg.showToolbox() %></span>
</div>
<div id="show-code-header" class="workspace-header workspace-header-button"><span><img src="<%= assetUrl('media/applab/blocks_glyph.gif') %>" class="blocks-glyph" /><i class="fa fa-code"></i><%= msg.showCodeHeader() %></span></div>
<% if (!data.readonlyWorkspace) { %>

This comment has been minimized.

Copy link
@bcjordan

bcjordan Mar 1, 2016

Contributor

I noticed this PR removes indentation inside most ejs if blocks, intentional?

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 1, 2016

Author Member

Ah, no, I missed that. Autoreformat on paste in RubyMine rearing its (normally useful) head. Any suggestions on fixing it's behavior in these sort of cases?

This comment has been minimized.

Copy link
@bcjordan

bcjordan Mar 1, 2016

Contributor

Wasn't able to find any options to fix this, looks like the JetBrains ejs plugin is just wrong.

image

Opened a ticket (don't see a GH repo for the integration)

@bcjordan

This comment has been minimized.

Copy link
Contributor

bcjordan commented Mar 1, 2016

LGTM! Love the separation and naming

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Mar 1, 2016

lgtm. nice progress! :)

islemaster added a commit that referenced this pull request Mar 1, 2016
React Wrapper Step 3: Split page.html.ejs along column divide
@islemaster islemaster merged commit 48ea6d2 into staging Mar 1, 2016
5 checks passed
5 checks passed
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0%) to 85.358%
Details
hound No violations found. Woof!
@islemaster islemaster deleted the react-wrapper-split-page-ejs branch Mar 1, 2016
deploy-code-org added a commit that referenced this pull request Mar 1, 2016
48ea6d2 Merge pull request #7051 from code-dot-org/react-wrapper-split-page-ejs (Brad Buchanan)
a373081 Indent 'if' blocks in new ejs files (Brad Buchanan)
93fd1bb Merge pull request #7055 from code-dot-org/selenim-selenium-1 (Ram Kandasamy)
9dde82a Automatically built. (Continuous Integration)
6994751 Fix typo: Selenim -> Selenium (Ram Kandasamy)
48ce748 Merge pull request #7050 from code-dot-org/copyrightSize (Brent Van Minnen)
b5bf638 Render resize bar as a ProtectedStatefulDiv (Brad Buchanan)
73b648a Automatically built. (Continuous Integration)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.