Initial commit of component render classes. #8

chriscox · 2016-10-20T21:06:04Z

No description provided.

chuga · 2016-10-20T21:11:15Z

src/ui/components/ComponentFactory.tsx

+ let variable: Variable = this.props.variable;
+
+ // Determine which type of component to render based on data type and content.
+ switch (variable.dataType) {


The developer can't override this default? I'm ok with that, but I thought we had decided that we wanted to provide that functionality. Maybe I'm wrong?

Yeah we will support that. Currently it's not added. I've created an issue to track it here: #10

chuga

I tried to review this but without knowing React there isn't much I can say about they way you're doing things. Can we get someone who knows React to review it?

appsforartists

I left my high-level architectural notes in #12. There are also a handful of code smells I noted in here (primarily in the Color control, since it's the first one I saw).

Happy to go over any of this with you. I'll be in MTV most of the week for UXU.

appsforartists · 2016-10-24T18:03:11Z

src/ui/DOMController.tsx

+
+import "./overlay/styles.less";
+import * as React from "react";
+import * as ReactDOM from "react-dom";


Why import * as React from 'react as opposed to just import React from 'react'?

Unfortunately React does not have a default export specified, and therefore your suggestion won't work in typescript. I'm importing as per Typescript documentation on working with React here: https://www.typescriptlang.org/docs/handbook/react-&-webpack.html

Good to know! I've always used Babel, which converts CommonJS modules to use default exports automatically.

appsforartists · 2016-10-24T18:10:35Z

src/ui/DOMController.tsx

+import { remixer } from "../core/Remixer";
+
+/**
+ * Interface for the properties assigned to a React class.


In both your interfaces, you say "to as React class." "to the DOMController React component" would be more accurate.

I question the value of adding comments that reiterate the same information the interface name does, but that's a personal persuasion - I don't know if there are Google rules around commenting.

appsforartists · 2016-10-24T18:13:37Z

src/ui/DOMController.tsx

+ * @interface
+ */
+interface ControllerState {
+ /** An array of Variables. */


This comment would be more helpful if you explained what the variables are and why you're keeping them in state. As it is now, it's literally just the type signature reiterated.

appsforartists · 2016-10-24T18:13:54Z

src/ui/DOMController.tsx

+}
+
+/**
+ * Avaialble routes to render.


"Available"

appsforartists · 2016-10-24T18:14:40Z

src/ui/DOMController.tsx

+
+/**
+ * A React component class that renders an MDL-styled card containing child
+ * components detemrined by its current Route.


"determined"

appsforartists · 2016-10-25T01:20:51Z

src/ui/components/ComponentFactory.tsx

+ let classNames: string = `mdl-list__item ${(controlLineCount > 1) ? "mdl-list__item--two-line" : ""}`;
+
+ return (
+ <li className={classNames}>


It's a bit weird to have an li in its own component; though based on the MDL naming, I'm guessing that's leaking out from MDL.

Each of these control components is rendered as a list item li within the ul of the parent component.

appsforartists · 2016-10-25T01:21:58Z

src/ui/components/Component.tsx

+ * @readonly.
+ * @return {string} The HTML element ID.
+ */
+ id: string;


How is this used? As I noted in another comment, reading values off of a React component feels strange.

appsforartists · 2016-10-25T01:24:08Z

src/ui/components/DropdownComponent.tsx

+ }
+
+ /** @override */
+ render() {


I reiterate all my feedback from the Color control. Seeing the same code here makes me wonder if it's factored correctly.

appsforartists · 2016-10-25T01:27:10Z

src/ui/components/RadioListComponent.tsx

+ // We will manually need to update the MDL component here to new state
+ // when receiving new props.
+ let selectedValue = this.props.variable.selectedValue;
+ let component = this.refs[this.id + "-" + selectedValue]["MaterialRadio"];


I believe this.refs is a deprecated syntax.

appsforartists · 2016-10-25T01:29:55Z

src/ui/overlay/overlay.html

+ <link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons">
+ <script src="https://code.getmdl.io/1.2.1/material.min.js"></script>
+ <script src="https://unpkg.com/react@15.3.2/dist/react.js"></script>
+ <script src="https://unpkg.com/react-dom@15.3.2/dist/react-dom.js"></script>


I trust Michael, but I doubt Google Security would feel good about us loading payloads off of domains we don't control.

Aren't you already importing React and ReactDOM via Webpack? Do you even need these here?

appsforartists · 2016-10-25T01:32:20Z

I always used " for quotes at past jobs, but have noticed that ' seems to be preferred at Google. It's not big-enough of a deal for me to fight, so I've been retraining myself to use '. But, if we want to decide as Material that we prefer ", I'm also 👍 with that.

chriscox

Initial pass of changes. More to come.

chriscox · 2016-10-25T15:57:08Z

src/ui/DOMController.tsx

+
+import "./overlay/styles.less";
+import * as React from "react";
+import * as ReactDOM from "react-dom";


Unfortunately React does not have a default export specified, and therefore your suggestion won't work in typescript. I'm importing as per Typescript documentation on working with React here: https://www.typescriptlang.org/docs/handbook/react-&-webpack.html

chriscox · 2016-10-25T15:59:14Z

src/ui/DOMController.tsx

+import { remixer } from "../core/Remixer";
+
+/**
+ * Interface for the properties assigned to a React class.


chriscox · 2016-10-25T16:07:58Z

src/ui/DOMController.tsx

+ * @interface
+ */
+interface ControllerState {
+ /** An array of Variables. */


chriscox · 2016-10-25T16:08:40Z

src/ui/DOMController.tsx

+}
+
+/**
+ * Avaialble routes to render.


chriscox · 2016-10-25T16:08:57Z

src/ui/DOMController.tsx

+
+/**
+ * A React component class that renders an MDL-styled card containing child
+ * components detemrined by its current Route.


chriscox · 2016-10-25T16:17:36Z

src/ui/DOMController.tsx

+ */
+ onMessageReceived(event: MessageEvent): void {
+ if (event.data === Messaging.type.ToggleVisibility) {
+ this.toggleVisiblity();


chriscox · 2016-10-25T16:37:10Z

src/ui/DOMController.tsx

+ */
+ updateRoute(event: Event) {
+ let newRoute: Route = (this.state.route === Route.Options) ? Route.Variables : Route.Options;
+ this.setState({variables: this.state.variables, route: newRoute});


Done. Apparently it is a known limitation to partially setState with typescript (see this issue: microsoft/TypeScript#4889). So to get around this I'm now making ControllerState properties optional. I'm ok with that as it now allows me to set only specific params of the state.

chriscox · 2016-10-25T17:17:15Z

src/ui/components/ColorSwatchComponent.tsx

+ componentWillReceiveProps() {
+ // We will manually need to update the MDL component here to new state
+ // when receiving new props.
+ this.state = {selectedValue: this.props.variable.selectedValue};


done.

TL;DR: I was able to completely remove all componentWillReceiveProps() method overrides from all component classes.

Longer explanation:
It was a legacy requirement no longer needed. Originally you could reset controls to their default via the restore button, and this method was called with updated props and needed to update state as well as modify the MDL components. No longer needed now because restore is done via a separate view, and therefore when going back to controls view it will naturally re-render the updated props.

chriscox · 2016-10-25T17:25:04Z

src/ui/components/ComponentFactory.tsx

+ * Interface for the properties of a Component class.
+ * @interface
+ */
+export interface ComponentProps { variable: Variable | Variable; }


done. Oops :)

chriscox · 2016-10-25T17:27:45Z

src/ui/components/ComponentFactory.tsx

+ let classNames: string = `mdl-list__item ${(controlLineCount > 1) ? "mdl-list__item--two-line" : ""}`;
+
+ return (
+ <li className={classNames}>


Each of these control components is rendered as a list item li within the ul of the parent component.

chriscox

Another pass at updates per feedback. Few issues remaining to be tackled next.

chriscox · 2016-10-26T01:57:41Z

src/ui/DOMController.tsx

+ case Route.Options:
+ return <Options variables={this.state.variables} />;
+ }
+ })()


chriscox · 2016-10-26T02:54:19Z

src/ui/components/ColorSwatchComponent.tsx

+
+ return (
+ <div>
+ {possibleValues.map((value: string, i: number) => (


done. Added new ColorSwatch component.

chriscox · 2016-10-26T02:54:53Z

src/ui/components/ColorSwatchComponent.tsx

+ {possibleValues.map((value: string, i: number) => (
+ <div className="rmx-color-item" style={ {backgroundColor: value} }
+ value={value} key={this.id + "-" + i} ref={value}
+ onClick={this.handleClick.bind(this, value)} data={value}>


done. Renamed to updateSelectedColor

chriscox · 2016-10-26T02:56:24Z

src/ui/components/ColorSwatchComponent.tsx

+ * Handles a click event on this component.
+ * @param {string} value The new selectedValue.
+ */
+ handleClick(value: string) {


done. This has been modified to updateSelectedColor method. Now each ColorSwatch component passes an onClick event to it. (See below for more info on ColorSwatch)

chriscox · 2016-10-26T02:57:15Z

src/ui/components/ColorSwatchComponent.tsx

+ /** @override */
+ render() {
+ let possibleValues: Array<string> = this.props.variable["possibleValues"];
+ let selectedValue: string = this.state.selectedValue;


chriscox · 2016-10-26T03:01:00Z

src/ui/components/Component.tsx

+ */
+ componentDidMount() {
+ for (let key in this.refs) {
+ window["componentHandler"].upgradeElement(this.refs[key]);


Unfortunately in this instance, componentHandler is added from the MDL library to window dynamically, and typescript complains if accessing this in another way besides dict.

chriscox · 2016-10-26T03:07:22Z

src/ui/overlay/overlay.html

+ <link rel="stylesheet" href="https://fonts.googleapis.com/icon?family=Material+Icons">
+ <script src="https://code.getmdl.io/1.2.1/material.min.js"></script>
+ <script src="https://unpkg.com/react@15.3.2/dist/react.js"></script>
+ <script src="https://unpkg.com/react-dom@15.3.2/dist/react-dom.js"></script>


appsforartists · 2016-10-26T15:22:11Z

src/ui/components/ComponentFactory.tsx

+ * property.
+ * @override
+ */
+ render() {


If this component doesn't have any state, you should make it a pure function:

function ControlListItem(props: ControlListItemProps) { // the stuff that's in render. }

appsforartists · 2016-10-26T15:23:27Z

Looks like you have "more changes to come." Let me know when you want me to look at it again. 😃

…tance.

… than Inheritance.

chriscox

Did a lot of refactoring here:

Following react guidelines of composition rather than inheritance.
Components renamed to Control
Controls no longer subclass a base class. Rather they are responsible for their entire layout.
A single Overlay class provides the scaffold for the controls. It also replaces the factory and simply determines which control to render based on their variable type.
Offloaded many reusable CSS class/ids to constants file.
Refactored constants to export more concisely based on grouped constant types.
Removed the options page. It previously showed only a Restore to Defaults button. We are reconsidering the concept of restore on all platforms. Therefore I've removed this options page and the routing required for it. Will reimplement following your guidelines specified in issue Make React architecture modular + unidirectional #12

chriscox · 2016-10-26T15:31:53Z

src/ui/DOMController.tsx

+ */
+ constructor(props: ControllerProps) {
+ super(props);
+ this.state = {variables: remixer.attachedInstance.variablesArray, route: Route.Variables};


chriscox · 2016-10-27T02:34:14Z

src/ui/components/Component.tsx

+ */
+ componentDidMount() {
+ for (let key in this.refs) {
+ window["componentHandler"].upgradeElement(this.refs[key]);


…omponent.

appsforartists · 2016-11-15T00:21:55Z

src/lib/Constants.ts

+ STRING: "string",
+};
+
+/** CSS class and id constants. */
 export const CSS = {


Might be a bit easier to read if you named it something camel-cased, like StyleClassName or CSSClassName.

appsforartists · 2016-11-15T00:33:53Z

src/ui/DOMController.tsx

 return (
- <div className="rmx-card-wide mdl-card mdl-shadow--6dp">
+ <div className="mdl-card mdl-shadow--6dp">


Why do you use string literals in some places and groups of CSSClassName.EXAMPLE in others?

appsforartists · 2016-11-15T00:37:44Z

src/ui/DOMController.tsx

 * @class
 * @extends React.Component
 */
 export class DOMController extends React.Component<ControllerProps, ControllerState> {
-
 state = {
 variables: remixer.attachedInstance.variablesArray,


I don't think this needs to be in state; looks like you had setState in an old version, but no longer. Also looks like you're only passing them through to Overlay, so maybe remove the reference in this component altogether.

appsforartists · 2016-11-15T00:39:52Z

src/ui/DOMController.tsx

-interface ControllerProps {
- wrapperElement: HTMLElement;
-}
+interface ControllerProps { wrapperElement: HTMLElement; }


Modifying a parent in a child is an antipattern in React; are you going to fix this in #12?

It would be pretty simple to change toggleVisibility to call this.setState({ isVisible: !this.state.isVisible }) and decide whether or not to return children from render using isVisible.

appsforartists · 2016-11-15T00:47:16Z

src/ui/controls/ColorSwatchControl.tsx

+ * @class
+ * @extends React.Component
+ */
+class ColorSwatch extends React.Component<ColorSwatchProps, void> {


If you don't have state, it should just be a function:

function ColorSwatch(props) { const { color, isSelected, onClick, } = props; return ( // ... JSX ); }

appsforartists · 2016-11-15T00:51:44Z

src/ui/controls/ColorSwatchControl.tsx

+ * @class
+ * @extends React.Component
+ */
+export class ColorSwatchControl extends React.Component<ControlInterface, ControlInterface> {


I'm no types expert, but wouldn't you want to have ColorControlInterface here, so you don't have to cast in componentWillMount et al?

The structure of all your controls feels similar, so my comments on this component echo across the others.

I was going for simplicity where I can pass in generic Variable to these controls, and only cast when needed for some specific property needed. But yeah agree its more confusing than its worth.

So, I've modified controlInterfaces.tsx file to now house a series of control interfaces (one for each type of Variable). This is what each control can now use as its interface.

appsforartists · 2016-11-15T00:53:17Z

src/ui/controls/ColorSwatchControl.tsx

+ */
+ updateSelectedColor(color: string) {
+ const { variable } = this.state;
+ variable.selectedValue = color;


This would be a good place to use Redux. Modifying external state in a component callback feels gross.

You can save that for #12.

Now these classes keep only a selectedValue state which is required for MDL to update the controls. And when the value changes, each control now calls a props callback method that gets handled in a single location in domcontroller.

appsforartists · 2016-11-15T00:57:04Z

src/ui/controls/ColorSwatchControl.tsx

+ */
+export class ColorSwatchControl extends React.Component<ControlInterface, ControlInterface> {
+ state = {
+ variable: this.props.variable,


Why are you storing a prop as state?

Seems like you could get rid of all the state entirely and just move the possibleValues.includes(selectedValue) check into render.

appsforartists · 2016-11-15T01:00:22Z

src/ui/overlay/Overlay.tsx

+ case VariableType.STRING:
+ if (!variable["possibleValues"]) {
+ Control = TextFieldControl;
+ } else if (variable["possibleValues"].length <= 2) {


So you could have a radio list with a single value?

Updated this. We are attempting to display the best control for the variable type. This section of code will change as we add controls and variables. We will also attempt to normalize these across platforms to be consistent. For now just manually setting this to 2 items for radio list.

appsforartists · 2016-11-15T01:00:56Z

src/ui/overlay/Overlay.tsx

- let RemixerControl = this.controlForVariable(variable);
- <RemixerControl variable={variable} key={variable.key} />
- /*this.controlForVariable(variable)*/
+ this.controlForVariable(variable)


Why not inline controlForVariable, so all your render logic is in one place?

chriscox

Latest round of updates (except changes for #12)

chriscox · 2016-11-15T18:02:23Z

src/ui/controls/ColorSwatchControl.tsx

+ * @class
+ * @extends React.Component
+ */
+class ColorSwatch extends React.Component<ColorSwatchProps, void> {


chriscox · 2016-11-15T18:05:55Z

src/ui/controls/ColorSwatchControl.tsx

+ * @class
+ * @extends React.Component
+ */
+export class ColorSwatchControl extends React.Component<ControlInterface, ControlInterface> {


I was going for simplicity where I can pass in generic Variable to these controls, and only cast when needed for some specific property needed. But yeah agree its more confusing than its worth.

So, I've modified controlInterfaces.tsx file to now house a series of control interfaces (one for each type of Variable). This is what each control can now use as its interface.

chriscox · 2016-11-15T18:48:37Z

src/ui/overlay/Overlay.tsx

+ case VariableType.STRING:
+ if (!variable["possibleValues"]) {
+ Control = TextFieldControl;
+ } else if (variable["possibleValues"].length <= 2) {


Updated this. We are attempting to display the best control for the variable type. This section of code will change as we add controls and variables. We will also attempt to normalize these across platforms to be consistent. For now just manually setting this to 2 items for radio list.

appsforartists · 2016-11-15T19:20:06Z

src/ui/controls/ColorSwatchControl.tsx

- * @class
- * @extends React.Component
+ * Returns a single color swatch displayed within the `ColorSwatchControl`.
+ * @param {ColorSwatchProps} props The color swath properties.


chriscox

@appsforartists Another update here that refactors the controls a bit.

appsforartists · 2016-11-17T19:10:10Z

src/ui/DOMController.tsx

+ReactDOM.render(
+ <div>
+ <DOMController
+ wrapperElement={element}


I still don't like that you're modifying the container. Any reason you can't remove the reference to wrapperElement and handle visibility in render?

appsforartists · 2016-11-17T19:11:31Z

src/ui/controls/ColorSwatchControl.tsx

+ const {
+ color,
+ isSelected,
+ onClick


It's a nice habit to put commas at the end of every line, to make it easier to maintain going forward. (Then, you can just duplicate and modify a line without worrying about the commas.)

appsforartists · 2016-11-17T19:17:40Z

src/ui/DOMController.tsx

+ * @param {Variable} variable The variable to update.
+ * @param {any} selectedValue The new selected value.
+ */
+ onUpdate = (variable: Variable, selectedValue: any): void => {


This isn't really an event listener. updateVariable would be a more accurate name.

appsforartists · 2016-11-17T19:21:43Z

src/ui/DOMController.tsx

-interface ControllerState { variables?: Array<Variable>; }
+interface ControllerProps {
+ wrapperElement: HTMLElement;
+ variables: Array<Variable>;


It's weird that you are specifying variables externally but onUpdate internally. What you really want to do is have every call to updateVariable update variables and then re-render with the updated version. That would remove the need for setState/forceUpdate inside your controls and let them be pure functions.

The idiomatic way to do that would be to have variables (and its values) be immutable, so each control could say

shouldComponentUpdate(nextProps) { return nextProps.variable !== this.props.variable }

appsforartists · 2016-11-17T19:23:49Z

src/ui/controls/ColorSwatchControl.tsx

 state = {
- variable: this.props.variable,
+ selectedValue: this.props.variable.selectedValue,


You aren't using this.state.selectedValue. As I mentioned in the DOMContainer comments, you ought to trigger the re-render up there, which lets you remove the state from all the controls.

FWIW, there is a forceUpdate method to accomplish what you're doing with setState here.

I don't want to trigger the re-render up in the DOMController because this forces every control to re-render (regardless if the control as been updated).

So I would rather trigger a render for individual controls when updated. Since I am not using selectedValue other than setting it to trigger a render, I can also use forceUpdate on each control here and forego the state. I'll update this accordingly.

appsforartists · 2016-11-17T19:25:12Z

src/ui/controls/DropdownControl.tsx

- this.setState({variable: variable});
- }
+ /** Handles the update event for this control. */
+ handleUpdate = (event: any) => {


Incidentally, this is an event listener. You could call it onItemClick.

appsforartists · 2016-11-17T19:30:57Z

src/ui/controls/DropdownControl.tsx

- this.setState({variable: variable});
- }
+ /** Handles the update event for this control. */
+ handleUpdate = (event: any) => {


The right way to type this in TypeScript is:

onItemClick = (event: Event) => { this.props.updateVariable( this.props.variable, (event.target as HTMLElement).dataset.value ); }

appsforartists · 2016-11-17T19:31:22Z

src/ui/controls/DropdownControl.tsx

 {possibleValues.map((value: string) => (
 <li className="mdl-menu__item" key={value}
- onClick={this.updatedSelectedValue.bind(this, value)}
+ onClick={this.handleUpdate}
 data={value}>{value}


This should be data-value = { value }

appsforartists · 2016-11-17T19:39:45Z

src/ui/overlay/Overlay.tsx

- this.controlForVariable(variable)
- ))}
+ <div className={CSS.MDL_LIST}>
+ {controls}


This could be inlined:

variables.map( variable => { const Control = this.controlForVariable(variable); return ( <Control variables = { variable } updateVariable = { this.props.updateVariable } key = { variable.key } /> ) } )

appsforartists · 2016-11-17T22:52:15Z

src/ui/DOMController.tsx

+ wrapperElement={element}
+ variables={remixer.attachedInstance.variablesArray}>
+ </DOMController>
+ </div>, element);


Responding to your question about how to externalize the re-rendering, this is the React way to do it:

let variables = remixer.attachedInstance.variablesArray; const overlayWrapper = document.getElementById(CSS.RMX_OVERLAY_WRAPPER); function redraw() { ReactDOM.render( <DOMController variables = { variables } />, overlayWrapper ); } variables.forEach( variable => variable.addCallback(redraw); ); redraw();

Of course, I cheated and used variables.forEach(variable.addCallback) because I know your in your current design, variables is a mutable array. In an idiomatic React app, your state is immutable, which makes rendering cheaper because you can use referential equality to avoid rendering parts that haven't changed: if oldProps === this.props, don't render that component.

…ts own `shouldComponentUpdate` method to determine if it should be re-rendered.

chriscox

Ok I think I've captured all the feedback (except #12 which will be subsequent change).

Now clones variables during update, allowing each control to handle its own shouldComponentUpdate method to determine if it should be re-rendered.

chriscox · 2016-11-21T18:35:52Z

src/ui/DOMController.tsx

+ wrapperElement={element}
+ variables={remixer.attachedInstance.variablesArray}>
+ </DOMController>
+ </div>, element);


…date.

appsforartists

Did these not get submitted before?

GitHub: how does it even?

appsforartists · 2016-11-21T19:38:11Z

src/core/variables/Variable.ts

+ * @param {VariableCallback} callback The callback to add.
+ */
+ addCallback(callback: VariableCallback): any {
+ this.callbacks.push(callback);


This should access the protected property callbacks_. get callbacks should return a copy, so it can't be modified externally.

Otherwise, why have getters/setters in the first place?

Fixed. This change was made during subsequent updates, and should have been included here.

appsforartists · 2016-11-21T19:52:15Z

src/ui/DOMController.tsx

 * @param {Variable} variable The variable to update.
 * @param {any} selectedValue The new selected value.
 */
- onUpdate = (variable: Variable, selectedValue: any): void => {
- variable.selectedValue = selectedValue;
+ updateVariable = (variable: Variable, selectedValue: any): void => {


This method doesn't belong in the component. All the state it references is stored in the top level.

appsforartists · 2016-11-21T19:53:27Z

src/ui/DOMController.tsx

- variable.selectedValue = selectedValue;
+ updateVariable = (variable: Variable, selectedValue: any): void => {
+ const index = variables.indexOf(variable);
+ let clonedVariable = variable.clone();


I left a comment in #19 about immutability clashing with the current representation of variables. I'm happy to come down to MTV next week and pair-program a better solution with you. 😃

appsforartists · 2016-11-21T19:54:35Z

src/ui/DOMController.tsx

-let element = document.getElementById(CSS.RMX_OVERLAY_WRAPPER);
-ReactDOM.render(
- <div>
+let variables = remixer.attachedInstance.variablesArray;


This code shouldn't be in the component - you should have a separate file, e.g. render.js, that handles your rendering. Component files should include only component code.

chriscox

Adds a render.tsx with logic that handles redrawing when variables update.

appsforartists · 2016-11-21T22:06:09Z

src/ui/render.tsx

+ variables[index] = clonedVariable;
+}
+
+let variables = remixer.attachedInstance.variablesArray;


This should be moved above updateVariable. Due to hoisting, the code you wrote will work, but it's easier to read if code is declared before it's used.

fixed. Made a quick cleanup pass.

appsforartists · 2016-11-21T22:11:34Z

src/ui/DOMController.tsx

@@ -86,25 +81,6 @@ export class DOMController extends React.Component<ControllerProps, void> {
 this.props.wrapperElement.classList.toggle(CSS.RMX_VISIBLE);


I feel like I've asked this in every review and haven't seen any comments/changes. Maybe the comments got lost in the GitHub UI?

Why are you mutating the DOM of an element this component doesn't control? Couldn't this just be:

toggleVisibility() { this.setState({ isVisible: !this.props.isVisible, }}) } render() { const { isVisible, variables, updateVariable, } = this.props; if (this.props.isVisible) { return ( // your DOM tree ) } else { return <div />; }

Yeah we chatted about this. Its part of #12. Just never made that change.

chriscox · 2016-11-21T22:28:10Z

@appsforartists Let me know if this looks ok now. There's just the overlay toggle issue, but as we've discussed that will be handled in a separate issue per #12.

appsforartists · 2016-11-21T22:39:49Z

I didn't realize you wanted to handle that separately; seemed small in relation to the other changes that have been made in this review. 😝

chriscox · 2016-11-21T22:42:46Z

Awesome thanks! Yeah #12 gradually grew smaller and smaller as changes went on. But I was just trying best not to mix issues so could handle that separately.

Thanks again for amazing exhaustive review! Fantastic feedback.

Initial commit of component render classes.

0638019

chuga reviewed Oct 20, 2016

View reviewed changes

chuga reviewed Oct 21, 2016

View reviewed changes

appsforartists self-assigned this Oct 24, 2016

appsforartists mentioned this pull request Oct 25, 2016

Make React architecture modular + unidirectional #12

Open

appsforartists suggested changes Oct 25, 2016

View reviewed changes

chriscox added 2 commits October 24, 2016 22:14

Merge branch 'develop' into feature/components

e5d96a8

First pass of updates per feedback.

8500f7e

chriscox commented Oct 25, 2016

View reviewed changes

Another pass of updates per feedback.

dad1c8b

chriscox commented Oct 26, 2016

View reviewed changes

appsforartists reviewed Oct 26, 2016

View reviewed changes

chriscox added 4 commits November 7, 2016 15:39

Refactors components to controls using Composition rather than Inheri…

3c4c4fe

…tance.

Finalizes refactor of components to controls using Composition rather…

2a0e894

… than Inheritance.

Updates constants formatting.

6bc9a3e

Fixes styles.less spacing/tabs.

db0fa11

chriscox commented Nov 9, 2016

View reviewed changes

Adds class commenting. Adds missing TextFieldControl. Removes SliderC…

939dbf7

…omponent.

chriscox mentioned this pull request Nov 9, 2016

Updates core variable interfaces due to Constants refactor. #16

Merged

chriscox added 2 commits November 10, 2016 12:30

Merge branch 'develop' into feature/components

f5d12f7

Cleans up a few comment and strings.

9ff18a0

appsforartists reviewed Nov 15, 2016

View reviewed changes

Updates control interfaces.

2840c31

chriscox commented Nov 15, 2016

View reviewed changes

appsforartists reviewed Nov 15, 2016

View reviewed changes

Refactors variable controls to call a prop callback method when updated.

9bde50b

chriscox commented Nov 17, 2016

View reviewed changes

appsforartists suggested changes Nov 17, 2016

View reviewed changes

appsforartists reviewed Nov 17, 2016

View reviewed changes

chriscox added 2 commits November 18, 2016 14:34

Work-in-progress.

be4a5d8

Now clones variables during update, allowing each control to handle i…

bef5a8e

…ts own `shouldComponentUpdate` method to determine if it should be re-rendered.

chriscox commented Nov 21, 2016

View reviewed changes

chriscox added 3 commits November 21, 2016 10:39

Resets package.json.

5d548ad

Merge branch 'develop' into feature/components

f791971

Adds a render.tsx with logic that handles redrawing when variables up…

95e2423

…date.

appsforartists reviewed Nov 21, 2016

View reviewed changes

chriscox commented Nov 21, 2016

View reviewed changes

appsforartists reviewed Nov 21, 2016

View reviewed changes

Fixes adding callbacks to internal _callbacks

fffa746

appsforartists reviewed Nov 21, 2016

View reviewed changes

Minor code cleanup.

e1ea4fc

appsforartists approved these changes Nov 21, 2016

View reviewed changes

chriscox merged commit 5dbbc93 into develop Nov 21, 2016

chriscox deleted the feature/components branch November 21, 2016 22:43

		@@ -86,25 +81,6 @@ export class DOMController extends React.Component<ControllerProps, void> {
		this.props.wrapperElement.classList.toggle(CSS.RMX_VISIBLE);

Initial commit of component render classes. #8

Initial commit of component render classes. #8

Conversation

chriscox commented Oct 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chuga left a comment

Choose a reason for hiding this comment

appsforartists left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appsforartists commented Oct 25, 2016

chriscox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

appsforartists commented Oct 26, 2016

chriscox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chriscox left a comment