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

Reduxify GameLab #7301

Merged
merged 10 commits into from Mar 16, 2016
Merged

Reduxify GameLab #7301

merged 10 commits into from Mar 16, 2016

Conversation

@islemaster
Copy link
Member

islemaster commented Mar 15, 2016

Does some boilerplate work to use Redux in GameLab like we've started using it in App Lab (including making some App Lab code common between the two). UI-wise this only gets as far as adding and interface mode switch to GameLab, but it's a good starting point for further animation tab work.

redux-gamelab

@@ -1,5 +1,6 @@
{
"and": "and",
"animationMode": "Animation",

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

Added to the common i18n file because this is where the code mode and design mode strings live.

action.type + ' action.');
}
});
return _.assign({}, state, action.props);

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

Preferring _.assign to $.extend because lodash doesn't require a browser context, so you can run the tests on this otherwise-pure function lightning-fast without phantom using mocha test/gamelab/reducersTest.js.

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 15, 2016

Contributor

Fine by me. In a world with ES6, I think we use Object.assign anyways (or possibly just object destructing/spread operator).

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

Whenever we make that move, I intend to go replace all uses of either of these with Object.assign 😁


return (
<div>
<GameLabVisualizationHeader/>

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

Note that we render a different GameLabVisualizationHeader here, but it's okay because it's stateless.

@islemaster

This comment has been minimized.

Copy link
Member Author

islemaster commented Mar 15, 2016

@cpirich

This comment has been minimized.

Copy link
Contributor

cpirich commented Mar 15, 2016

LGTM - but my react knowledge is very limited 😄

<GameLabView
renderCodeWorkspace={renderCodeWorkspace}
renderVisualizationColumn={renderVisualizationColumn}
onMount={onMount} />

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 15, 2016

Contributor

Note: If we get to the point that renderCodeWorkspace/renderVisualization column return React components, we should provide those as children instead of props, i.e.

<GameLabView onMount={onMount}>
  {renderCodeWorkspace()}
  {renderVisualizationColumn}
</GameLab>

I don't think we're there yet though. I wonder if we should use a different word than render to make it clear that this is a non-React rendering?

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

For now, these are functions passed around explicitly for the purpose of doing one-time non-React EJS rendering into ProtectedStatefulDivs. If they ever return React components, we probably don't need to pass them as props anymore.

I'll come up with a name that makes it more obvious what these are for.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

Done, in a fairly large renames-only change to stop saying "render" when it might be confused with React rendering.

var codeModeStyle = {};
if (this.props.interfaceMode !== GameLabInterfaceMode.CODE) {
codeModeStyle.display = 'none';
}

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 15, 2016

Contributor

should we make the value of display explicit in the case where these are the same?

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

I'm not sure which is preferred; I thought about both and decided I liked not setting any CSS properties on this container when I didn't want to hide it. If you prefer the following I'm happy to switch:

var codeModeStyle = {
  display: this.props.interfaceMode === GameLabInterfaceMode.CODE ? 'block' : 'none'
};

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 15, 2016

Contributor

I think how you have it in this comment is how I would have written, but as I think about this, I dont feel strongly about it at all. I'm fine with either approach.

renderAnimationMode: function () {
// Don't render animation mode when we're not in that mode (pure React!)
if (this.props.interfaceMode !== GameLabInterfaceMode.ANIMATION) {
return undefined;

This comment has been minimized.

Copy link
@Bjvanminnen

Bjvanminnen Mar 15, 2016

Contributor

Do you like this better than just return;? Feels a little weird to me, but I'm not sure that I'm against it.

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

I do prefer it in a method that would ever return a value - a bare return; in a method that can return a value always looks like a mistake to me.

* @private {Store}
* @see http://redux.js.org/docs/basics/Store.html
*/
this.reduxStore_ = createStore(gamelabReducer);

This comment has been minimized.

Copy link
@bcjordan

bcjordan Mar 15, 2016

Contributor

Curious—design-wise is it the case that applab & gamelab's AppName.stores might move upwards to StudioApp.prototype or somewhere else shared at some point in the future?

This comment has been minimized.

Copy link
@islemaster

islemaster Mar 15, 2016

Author Member

It's possible it would move upward at some point, but I'm not sure I'm convinced we're there yet. I don't intend to backport redux into all of our classic puzzle types (I'm not sure the work is justified at this point) and applab/gamelab don't have a common ancestor that's not also shared with everything.

On the other hand, I feel like redux is lightweight and encapsulated enough that it might be better not to centralize it. IMO one of our architectural problems is that we've been defining app types by inheritance rather than composition, which encourages duplication of any code that's needed by some but not all apps. I'm trying to push us away from that with the top-level React work - apps should have unique top-level glue code, instantiating reusable components and tying them together.

The singular redux store feels (to me) like something that lives in that top glue-code layer - a page should only have one store, driven by one (composed) reducer that's probably unique to that app type. The subreducers that make up the app-level reducer are probably reused though, which should minimize duplication in redux code too.

@Bjvanminnen

This comment has been minimized.

Copy link
Contributor

Bjvanminnen commented Mar 15, 2016

lgtm

@bcjordan

This comment has been minimized.

Copy link
Contributor

bcjordan commented Mar 15, 2016

LGTM! Nice testing 👍

islemaster added 2 commits Mar 15, 2016
Removes the word 'render' from React-touching stuff where we're talking about markup generation that isn't done by React, to avoid confusion.

s/renderContents/contentFunction on ProtectedStatefulDiv
s/renderCodeApp/generateCodeAppHtml(FromEjs)?
s/renderCodeWorkspace/generateCodeWorkspaceHtml(FromEjs)?
s/renderVisualizationColumn/generateVisualizationColumnHtml(FromEjs)?
islemaster added a commit that referenced this pull request Mar 16, 2016
Reduxify GameLab
@islemaster islemaster merged commit 6dc6111 into staging Mar 16, 2016
4 of 5 checks passed
4 of 5 checks passed
coverage/coveralls Coverage pending from Coveralls.io
Details
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
hound No violations found. Woof!
@islemaster islemaster deleted the reduxify-gamelab branch Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.