Skip to content
This repository has been archived by the owner on Jun 3, 2022. It is now read-only.

Initial commit of component render classes. #8

Merged
merged 20 commits into from
Nov 21, 2016
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
0638019
Initial commit of component render classes.
chriscox Oct 20, 2016
e5d96a8
Merge branch 'develop' into feature/components
chriscox Oct 25, 2016
8500f7e
First pass of updates per feedback.
chriscox Oct 25, 2016
dad1c8b
Another pass of updates per feedback.
chriscox Oct 26, 2016
3c4c4fe
Refactors components to controls using Composition rather than Inheri…
chriscox Nov 7, 2016
2a0e894
Finalizes refactor of components to controls using Composition rather…
chriscox Nov 9, 2016
6bc9a3e
Updates constants formatting.
chriscox Nov 9, 2016
db0fa11
Fixes styles.less spacing/tabs.
chriscox Nov 9, 2016
939dbf7
Adds class commenting. Adds missing TextFieldControl. Removes SliderC…
chriscox Nov 9, 2016
f5d12f7
Merge branch 'develop' into feature/components
chriscox Nov 10, 2016
9ff18a0
Cleans up a few comment and strings.
chriscox Nov 10, 2016
2840c31
Updates control interfaces.
chriscox Nov 15, 2016
9bde50b
Refactors variable controls to call a prop callback method when updated.
chriscox Nov 17, 2016
be4a5d8
Work-in-progress.
chriscox Nov 18, 2016
bef5a8e
Now clones variables during update, allowing each control to handle i…
chriscox Nov 21, 2016
5d548ad
Resets package.json.
chriscox Nov 21, 2016
f791971
Merge branch 'develop' into feature/components
chriscox Nov 21, 2016
95e2423
Adds a render.tsx with logic that handles redrawing when variables up…
chriscox Nov 21, 2016
fffa746
Fixes adding callbacks to internal _callbacks
chriscox Nov 21, 2016
e1ea4fc
Minor code cleanup.
chriscox Nov 21, 2016
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
171 changes: 171 additions & 0 deletions src/ui/DOMController.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
/** @license
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy
* of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import { Constants as CONST } from "../lib/Constants";
import { Messaging } from "../lib/Messaging";
import { Options } from "./overlay/Options";
import { Overlay } from "./overlay/Overlay";
import { Variable } from "../core/variables/Variable";
import { remixer } from "../core/Remixer";

/**
* Interface for the properties assigned to a React class.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

* @interface
*/
interface ControllerProps {
wrapperElement: HTMLElement;
}

/**
* Interface for the state of a React class.
* @interface
*/
interface ControllerState {
/** An array of Variables. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

variables: Array<Variable>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}


/** The current route to render. */
route: Route;
}

/**
* Avaialble routes to render.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Available"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

*/
enum Route {
Variables,
Options
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"determined"

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

*
* As determined by the Route, the card content can be either:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Depending on the Route" would be a bit easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

* 1) The overlay with configurable control components per assigned Variables.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The language could be simpler: "Controls to tweak the values for each Variable"

This comment seems like it would fit the Route enum better. Is it the DOMController's job to know what its children are?

* 2) An options page used to configure Remixer session properties.
*
* @class
* @extends React.Component
*/
export class DOMController extends React.Component<ControllerProps, ControllerState> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component it trying to do too many things. See my advice in #12.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! After we land this I will do some refactoring per #12.


/**
* Default constructor.
* @constructor
* @type {DOMController}
*/
constructor(props: ControllerProps) {
super(props);
this.state = {variables: remixer.attachedInstance.variablesArray, route: Route.Variables};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in TypeScript you can set state as an instance property, and elide the constructor altogether; just delete the rest of the constructor body and leave this in its place.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I'm following your suggestion here. I'm setting state initially in the constructor based on guidelines here: https://facebook.github.io/react/docs/state-and-lifecycle.html. Can you clarify what your request is here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class Thing extends React.Component {
  state = {
    something: 123,
  }

  // Now you don't need a custom constructor
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

}

/**
* The component will mount. Lets register for messaging and key events.
* @override
*/
componentDidMount() {
Messaging.register(this.onMessageReceived.bind(this));
this.addKeyListener();
}

/**
* The component will unmount. Lets unregister.
* @override
*/
componentWillUnmount() {
Messaging.unregister();
this.removeKeyListener();
}

/**
* Handles receiving of window message.
* @param {MessageEvent} event The received message event.
*/
onMessageReceived(event: MessageEvent): void {
if (event.data === Messaging.type.ToggleVisibility) {
this.toggleVisiblity();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

toggleVisibility is misspelled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}
}

/** Adds a key listener. */
addKeyListener(): void {
document.addEventListener(CONST.KEY_EVENT_DOWN, (e: KeyboardEvent) => {
if (e.keyCode === CONST.KEY_CODE_ESC) {
this.toggleVisiblity();
}
});
}

/** Removes a key listener. */
removeKeyListener(): void {
document.removeEventListener(CONST.KEY_EVENT_DOWN);
}

/** Toggles the Remixer overlay visibility. */
toggleVisiblity() {
this.props.wrapperElement.classList.toggle(CONST.CSS_CLASS_VISIBLE);
}

/**
* Updates the state to new route.
* @param {Event} event The route to update to.
*/
updateRoute(event: Event) {
let newRoute: Route = (this.state.route === Route.Options) ? Route.Variables : Route.Options;
this.setState({variables: this.state.variables, route: newRoute});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setState only modifies the keys you send. This could be

const currentRoute = this.state.route;

this.setState(
  {
    route: currentRoute === Route.Variables
      ? Route.Options
      : Route.Variables,
  }
)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

/**
* Renders the component via React.
* @override
*/
render() {
return (
<div className="rmx-card-wide mdl-card mdl-shadow--6dp">
<div className="mdl-card__title" ref="myInput">
<h2 className="mdl-card__title-text">Remixer</h2>

</div>
<div className="mdl-card__menu">
<button className="mdl-button mdl-button--icon mdl-js-button mdl-js-ripple-effect"
onClick={this.updateRoute.bind(this)}>
<i className="material-icons">menu</i>
</button>
</div>
<div className="mdl-card__supporting-text mdl-card__actions mdl-card--border">
{(() => {
switch (this.state.route) {
case Route.Variables:
return <Overlay variables={this.state.variables} />;
case Route.Options:
return <Options variables={this.state.variables} />;
}
})()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is allocating a new function and calling it on every render for what is effectively an if statement or ternary:

const Route = this.state.route === Route.Options
  ? Options
  : Overlay;

return (
  // ...
  <Route variables = { this.state.variables } />

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

}
</div>
</div>
);
}
}

/**
* Renders the DOMController component to the overlay wrapper element.
*/
let element = document.getElementById(CONST.ID_OVERLAY_WRAPPER);
ReactDOM.render(<div><DOMController wrapperElement={element} /></div>, element);
72 changes: 72 additions & 0 deletions src/ui/components/ColorSwatchComponent.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/** @license
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy
* of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

import * as React from "react";
import { Component } from "./Component";

/**
* A React component class that renders a color swatch picker component.
* @class
* @extends Component
*/
export class ColorSwatchComponent extends Component {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You typed your props in the DOMContainer component; why not here?

Wait, I see now that you're extending a custom Component class. That's confusingly similar to React.Component, so RemixerControl would be a better name. Still, React strongly favors composition over inheritance. I've never seen a React component extend from another one. Can you model this without the extends RemixerControl?


/** @override */
static numberOfLines: number = 2;

/** @override */
get id(): string {
return `rmx-swatch-${this.props.variable.key}`;
}

/** @override */
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};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should never modify state directly; that's what the setState method is for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a property, not a method, to ensure that this is bound correctly. See toggleOptions in #12.

Otherwise, you're allocating a new function every time a circle is rendered.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

this.updateSelectedValue(value);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dictionary notation is frowned on in Google Style. This could be this.props.variable.possibleValues; though without knowing the shape of the object, I don't know why variable is even in there.

let selectedValue: string = this.state.selectedValue;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Destructuring is your friend:

let {
  selectedValue,
} = this.state;

Makes it really easy to pull a bunch of values out of state/props.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.


// Ensure selected value is choosable from possible values.
if (possibleValues.indexOf(selectedValue) === -1) {
possibleValues.push(selectedValue);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're modifying the props you get passed in? That makes it hard for the rest of the code to reason about possibleValues, and could introduce hard-to-find bugs.

Can you break this out into its own method and call it when selectedValue changes?

storeValuesInState() {
  const {
    possibleValues,
    selectedValue,
  } = this.state;

  if (!possibleValues.includes(selectedValue)) {
    this.setState(
      {
        possibleValues: [...possibleValues, selectedValue],
      }
    );
  }
}

};

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a lot easier to read if you had a ColoredCircle component:

<ColoredCircle
  radius = { 56 }
  color = { value }
  onClick = { this.onClick }
>
  {
    selectedValue === value
      ? <MaterialIcon>
          check
        </MaterialIcon>
      : null
  }
</ColoredCircle>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Added new ColorSwatch component.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handleClick is not the name I'd expect for a function with the signature (color). updateSelectedColor is a better name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done. Renamed to updateSelectedColor

{ (selectedValue === value) ? <i className="material-icons">check</i> : "" }
</div>
))}
</div>
);
}
}
87 changes: 87 additions & 0 deletions src/ui/components/Component.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
/** @license
* Copyright 2016 Google Inc. All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License"); you may not
* use this file except in compliance with the License. You may obtain a copy
* of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
* WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
* License for the specific language governing permissions and limitations
* under the License.
*/

import * as React from "react";
import { ComponentFactory, ComponentProps } from "./ComponentFactory";

/**
* Interface for a Component class.
* @interface
*/
interface ComponentType {
id: string;
updateSelectedValue(newValue: any): void;
}

/**
* Interface for the state of a Component class.
* @interface
*/
interface ComponentState {
selectedValue: any;
}

/**
* A React component class that Remixer control components should subclass.
* @class
* @extends React.Component
* @implements Component
*/
export class Component extends React.Component<ComponentProps, ComponentState> implements ComponentType {
constructor(props: ComponentProps) {
super(props);
this.state = {selectedValue: props.variable.selectedValue};
}

/**
* Number of lines to render this component. Defaults to 1.
* @override
* @static
* @type {number}
*/
static numberOfLines: number = 1;

/**
* The HTML id for this component. Subclasses should override and provide a
* unique id in the form of `rmx-{component-name}-key`.
*
* As an example: rmx-switch-title.
*
* @readonly.
* @return {string} The HTML element ID.
*/
id: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


/**
* The component will mount. Dynamicly generate MDL elements require
* calling `upgradeElement` for each component.
* @override
*/
componentDidMount() {
for (let key in this.refs) {
window["componentHandler"].upgradeElement(this.refs[key]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my note about dictionary access.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@appsforartists appsforartists Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're using TypeScript, you should probably type these references. I think it's

interface ComponentHandler {
  upgradeElement: (element:Element) => void;
}
declare componentHandler: ComponentHandler;

Maybe @traviskaufman already has type declarations you can use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

}
}

/**
* Updates the state and selected value.
* @param {any} newValue The new value to update to.
*/
updateSelectedValue(newValue: any) {
this.setState({selectedValue: newValue});
this.props.variable.selectedValue = newValue;
}
}
Loading