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 4: Extract PlaySpaceHeader from ejs #7063

Merged
merged 12 commits into from Mar 2, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Mar 1, 2016

Make PlaySpaceHeader render by the top-level component instead of directly by designMode.js.

To do this, App Lab gets its own top-level component AppLabView - we don't want to put this feature on our standard AppView component because most puzzle types won't ever need to render this header.

A few general transformations were needed to make this possible.

  1. Extracted Applab.render() so we can update the whole view any time, not just once inside Applab.init(). Our ProtectedStatefulDivs are doing their job for the first time!
  2. That render method needs to query the current state (for now) so a few design mode properties got accessors to help with that.
  3. AppLabView ends up taking all of PlaySpaceHeader's properties so it can pass them along. For now, that means lots of scaffolding. I hope to clean this up soon with Redux.

I thought about introducing Redux as well to clean up this PR, but it's going to require updating React to v0.14.x which will be a big change on its own, so I'm stopping here for now and will consider that the next step.

Some additional cleanup:

  • Certain logic (e.g. whether to show/hide the view data button) got moved into AppLabView and out of applab.js.
  • Cleaned up the last vestiges of designToggleRow (former name of PlaySpaceHeader).
  • Removed some methods that got reduced to simple pass-throughs.

Next steps:

  • Update React from v0.13 to v0.14
  • Start using react-redux, move props into the Redux store one at a time
    • activeScreenId would be a good place to start
    • Moving the code mode / design mode state into Redux (and out of the toggle) would be a big win.
    • Level properties (which shouldn't change after initial render) to avoid lots of passthrough for these effectively global properties.

Step 4 from this design document.

onCodeModeButton: Applab.onCodeModeButton,
onViewDataButton: Applab.onViewData,
onScreenChange: designMode.changeScreen,
onScreenCreate: designMode.createScreen

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 1, 2016

Contributor

Should all of these on* methods not be in reactInitialProps_ since presumably they wont be changing on rerender? Or is the goal here just to show which things we might be able to move into a redux store?

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 1, 2016

Author Member

I just put everything that got defined in Applab.init() into reactInitialProps_ - maybe I should have named it reactDefinedOnInitProps_. The rest of these might be defined as "things I can get at anytime." Eventually I'd like both these and the reactInitialProps_ to get absorbed into redux, this just seemed like an appropriate intermediate step.

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Mar 1, 2016

Approach generally lgtm

@islemaster islemaster changed the title [WIP] React Wrapper Step 4: Extract PlaySpaceHeader from ejs React Wrapper Step 4: Extract PlaySpaceHeader from ejs Mar 2, 2016
@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Mar 2, 2016

@bcjordan

This comment has been minimized.

Copy link
Contributor

bcjordan commented Mar 2, 2016

LGTM!

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Mar 2, 2016

lgtm

islemaster added a commit that referenced this pull request Mar 2, 2016
…spaceheader

React Wrapper Step 4: Extract PlaySpaceHeader from ejs
@islemaster islemaster merged commit 8958348 into staging Mar 2, 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-connect-playspaceheader branch Mar 2, 2016
deploy-code-org added a commit that referenced this pull request Mar 2, 2016
8958348 Merge pull request #7063 from code-dot-org/react-wrapper-connect-playspaceheader (Brad Buchanan)
ac37f51 Merge pull request #7081 from code-dot-org/activityMonitorPeriod (ashercodeorg)
ed77521 Changes activity-monitor to run every two hours. Changes how and where messages are logged (Asher Kach)
ea615a9 Merge pull request #7059 from code-dot-org/stagingRetentionDashboard (ashercodeorg)
29e7dbe Merge pull request #7079 from code-dot-org/excluded-levels (Tanya Parker)
8eb83a7 reorder excluded level to be in increasing order (Tanya Parker)
5aa5cfd exclude challenge levels from activity monitor (Tanya Parker)
2ca0238 Automatically built. (Continuous Integration)
0ea93f6 Merge pull request #7058 from code-dot-org/keep-coding-more (Ram Kandasamy)
80fe5d7 Merge pull request #7075 from code-dot-org/fix-farmer-map-editor (Elijah Hamovitz)
140586d Merge pull request #7074 from code-dot-org/removepromoteaction (Brendan Reville)
16ae49a Allow farmer map editor to create valueless cells (Elijah Hamovitz)
fb5cd5e Remove California action from state /promote. (Brendan Reville)
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.