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 2: Break RotateContainer and cleardiv out of EJS #7026

Merged
merged 9 commits into from Feb 29, 2016

Conversation

Projects
None yet
3 participants
@islemaster
Member

islemaster commented Feb 27, 2016

Start removing content from page.html.ejs.

  • Makes #rotateContainer into a component and renders it using React.
  • Renders .clear div using React.
  • Extracts top-level render parameters into variables, per Brent's suggestion in #7000.
  • Updates NetSim to use AppView.jsx for its top-level rendering and removes config.html entirely. NetSim will need it's own top-level jsx at some point, but hasn't diverged yet.

TODO before merge:

  • We show/hide the rotate container in our own code sometimes - move that logic to React.
  • Make sure any reference to config.html in Ruby code has been removed.

Step 2 from this design document.

@islemaster

This comment has been minimized.

Show comment
Hide comment
Member

islemaster commented Feb 27, 2016

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Feb 27, 2016

Member

Once again, diff ignoring whitespace is recommended.

Member

islemaster commented Feb 27, 2016

Once again, diff ignoring whitespace is recommended.

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Feb 29, 2016

Member

Okay, pretty confident config.html has never been set from dashboard code and there's nothing to clean up.

No occurrences of ^\s*html\s*$ found in dashboard.

(If dashboard was setting config.html I'd expect to find an entry somewhere like the serialized_attrs in blockly.rb)

Member

islemaster commented Feb 29, 2016

Okay, pretty confident config.html has never been set from dashboard code and there's nothing to clean up.

No occurrences of ^\s*html\s*$ found in dashboard.

(If dashboard was setting config.html I'd expect to find an entry somewhere like the serialized_attrs in blockly.rb)

@Bjvanminnen

This comment has been minimized.

Show comment
Hide comment
@Bjvanminnen

Bjvanminnen Feb 29, 2016

Contributor

lgtm

Contributor

Bjvanminnen commented Feb 29, 2016

lgtm

@Hamms

This comment has been minimized.

Show comment
Hide comment
@Hamms

Hamms Feb 29, 2016

Contributor

LGTM, pending that remaining TODO

Contributor

Hamms commented Feb 29, 2016

LGTM, pending that remaining TODO

islemaster added some commits Feb 29, 2016

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Feb 29, 2016

Member

@Hamms @Bjvanminnen PTAL:

  • Responded to CR feedback
  • Moved RotateContainer show/hide to React rendering.

The latter step has actually duplicated knowledge of whether to require landscape mode across the different apps. I'm not sure where to deduplicate it again.

  1. StudioApp could get a static requireLandscape(config) helper
  2. AppView could require the share and config props separately, and own the requireLandscape logic internally
  3. Dashboard could compute requireLandscape itself and put it on the config object, which would get passed bare to AppView
  4. Most dramatic: We could move top-level rendering responsibility back into StudioApp but have the specific app pass a content component (where the ProtectedStatefulDiv goes now).

Thoughts? I'm learning toward option 2 right now, but not sure any are worth doing yet - it's possible the next step (splitting page.html.ejs) will bring more clarity to this discussion.

Member

islemaster commented Feb 29, 2016

@Hamms @Bjvanminnen PTAL:

  • Responded to CR feedback
  • Moved RotateContainer show/hide to React rendering.

The latter step has actually duplicated knowledge of whether to require landscape mode across the different apps. I'm not sure where to deduplicate it again.

  1. StudioApp could get a static requireLandscape(config) helper
  2. AppView could require the share and config props separately, and own the requireLandscape logic internally
  3. Dashboard could compute requireLandscape itself and put it on the config object, which would get passed bare to AppView
  4. Most dramatic: We could move top-level rendering responsibility back into StudioApp but have the specific app pass a content component (where the ProtectedStatefulDiv goes now).

Thoughts? I'm learning toward option 2 right now, but not sure any are worth doing yet - it's possible the next step (splitting page.html.ejs) will bring more clarity to this discussion.

@Bjvanminnen

This comment has been minimized.

Show comment
Hide comment
@Bjvanminnen

Bjvanminnen Feb 29, 2016

Contributor

2 sounds reasonable to me, but punting on all for the time being also seems fine

Contributor

Bjvanminnen commented Feb 29, 2016

2 sounds reasonable to me, but punting on all for the time being also seems fine

@islemaster

This comment has been minimized.

Show comment
Hide comment
@islemaster

islemaster Feb 29, 2016

Member

Yeah, punting on all for now.

Member

islemaster commented Feb 29, 2016

Yeah, punting on all for now.

islemaster added a commit that referenced this pull request Feb 29, 2016

Merge pull request #7026 from code-dot-org/react-wrapper
React Wrapper Step 2: Break RotateContainer and cleardiv out of EJS

@islemaster islemaster merged commit 031179d into staging Feb 29, 2016

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.04%) to 85.776%
Details
hound No violations found. Woof!

@islemaster islemaster deleted the react-wrapper branch Feb 29, 2016

deploy-code-org added a commit that referenced this pull request Feb 29, 2016

Automatically built.
031179d Merge pull request #7026 from code-dot-org/react-wrapper (Brad Buchanan)
0008af6 Move logic for hiding/omitting RotateContainer to React. (Brad Buchanan)
17c53e1 Merge pull request #7041 from code-dot-org/levelbuilder (David Bailey)
9b882ca levelbuilder content changes (-dave) (Continuous Integration)
69b9465 staging content changes (-dave) (Continuous Integration)
b07e6e1 Merge pull request #7028 from code-dot-org/rake_build_symlinks (Will Jordan)
27aa218 Remove unneeded assetUrl option to netsim page.html.ejs (Brad Buchanan)
36f2e4d Merge pull request #7003 from code-dot-org/tableMetadata (Brent Van Minnen)
140c8c7 Remove extra whitespace in RotateContainer.jsx (Brad Buchanan)
7f184f6 Move RotateContainer import to end of common.scss (Brad Buchanan)

balderdash added a commit that referenced this pull request Mar 1, 2016

Automatically built.
031179d Merge pull request #7026 from code-dot-org/react-wrapper (Brad Buchanan)
0008af6 Move logic for hiding/omitting RotateContainer to React. (Brad Buchanan)
17c53e1 Merge pull request #7041 from code-dot-org/levelbuilder (David Bailey)
9b882ca levelbuilder content changes (-dave) (Continuous Integration)
69b9465 staging content changes (-dave) (Continuous Integration)
b07e6e1 Merge pull request #7028 from code-dot-org/rake_build_symlinks (Will Jordan)
27aa218 Remove unneeded assetUrl option to netsim page.html.ejs (Brad Buchanan)
36f2e4d Merge pull request #7003 from code-dot-org/tableMetadata (Brent Van Minnen)
140c8c7 Remove extra whitespace in RotateContainer.jsx (Brad Buchanan)
7f184f6 Move RotateContainer import to end of common.scss (Brad Buchanan)

Hamms pushed a commit to code-dot-org/artist that referenced this pull request Apr 23, 2018

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