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

Initial commit of component render classes. #8

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
70 changes: 49 additions & 21 deletions src/lib/Constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,30 +17,58 @@
/** A set of Constants. */
export class Constants {

/** Keyboard constants. */
static KEY_EVENT_DOWN = "keydown";
static KEY_CODE_ESC = 27;
// /** Keyboard constants. */
// static KEY_EVENT_DOWN = "keydown";
// static KEY_CODE_ESC = 27;

/** HTML IDs. */
static ID_OVERLAY_FRAME = "rmx-overlay-frame";
static ID_OVERLAY_WRAPPER = "rmx-overlay-wrapper";
static ID_CONTROL_SWITCH = "rmx-switch";

/** Remixer storage key. */
static STORAGE_KEY_REMIXER = "__Remixer__";

/** Variable types. */
static VARIABLE_TYPE_BOOLEAN = "boolean";
static VARIABLE_TYPE_COLOR = "color";
static VARIABLE_TYPE_NUMBER = "number";
static VARIABLE_TYPE_RANGE = "range";
static VARIABLE_TYPE_STRING = "string";

}

/** Keycode constants. */
export const KeyCode = {
ESC: 27
};

/** Key event constants. */
export const KeyEvent = {
DOWN: "keydown"
};

/** Storage keys constants. */
export const StorageKey = {
REMIXER: "__Remixer__"
};

/** Variable types constants. */
export const VariableType = {
BOOLEAN: "boolean",
COLOR: "color",
NUMBER: "number",
RANGE: "range",
STRING: "string",
};

/** CSS class and id constants. */
export const CSS = {
Copy link
Member

Choose a reason for hiding this comment

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

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

VISIBLE: "visible",
MDL_LIST_ITEM: "mdl-list__item",
MDL_PRIMARY: "mdl-list__item-primary-content",
MDL_SECONDARY: "mdl-list__item-secondary-content",
RMX_SWITCH: "rmx-switch"
MDL_LIST: "mdl-list",
MDL_LIST_ITEM: "mdl-list__item",
MDL_PRIMARY: "mdl-list__item-primary-content",
MDL_SECONDARY: "mdl-list__item-secondary-content",
MDL_TWO_LINE: "mdl-list__item--two-line",

RMX_OVERLAY_FRAME: "rmx-overlay-frame",
RMX_OVERLAY_WRAPPER: "rmx-overlay-wrapper",
RMX_VISIBLE: "rmx-visible",
RMX_SELECTED_VALUE: "rmx-selected-value",
RMX_COLOR_SWATCH: "rmx-color-swatch",
RMX_COLOR_SWATCH_ITEM: "rmx-color-swatch-item",
RMX_DROPDOWN: "rmx-dropdown",
RMX_RADIO_LIST: "rmx-radio-list",
RMX_RADIO_LIST_ITEM: "rmx-radio-list-item",
RMX_SLIDER: "rmx-slider",
RMX_SLIDER_MIN: "rmx-slider-min-value",
RMX_SLIDER_MAX: "rmx-slider-max-value",
RMX_SWITCH: "rmx-switch",
RMX_TEXTFIELD: "rmx-textfield",
};
92 changes: 16 additions & 76 deletions src/ui/DOMController.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,8 @@
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, CSS } from "../lib/Constants";
import { KeyCode, KeyEvent, CSS } 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";
Expand All @@ -28,69 +27,35 @@ import { remixer } from "../core/Remixer";
* Interface for the properties assigned to the DOMController component.
* @interface
*/
interface ControllerProps {
wrapperElement: HTMLElement;
}
interface ControllerProps { wrapperElement: HTMLElement; }
Copy link
Member

Choose a reason for hiding this comment

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

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.


/**
* Interface for the state of the DOMController component.
* @interface
*/
interface ControllerState {
/**
* The DOMController will render a control component for each of the variables
* of this array.
* @type {Array<Variable>}
*/
variables?: Array<Variable>;

/**
* The current route to render.
* @type {Route}
*/
route?: Route;
}

/**
* Available routes to render.
*/
enum Route {
Variables,
Options
}
interface ControllerState { variables?: Array<Variable>; }

/**
* A React component class that renders an MDL-styled card containing child
* components determined by its current Route.
*
* Depending on the Route, the card content can be either:
* 1) The overlay with configurable control components per assigned Variables.
* 2) An options page used to configure Remixer session properties.
*
* Renders an MDL card-styled overlay containing a child control for each
* variable.
* @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.


state = {
variables: remixer.attachedInstance.variablesArray,
Copy link
Member

Choose a reason for hiding this comment

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

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.

route: Route.Variables
};

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

/**
* The component will unmount. Lets unregister.
* @override
*/
/** @override */
componentWillUnmount() {
// Unregister for messaging and key events.
Messaging.unregister();
this.removeKeyListener();
}
Expand All @@ -107,56 +72,31 @@ export class DOMController extends React.Component<ControllerProps, ControllerSt

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

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

/** Toggles the Remixer overlay visibility. */
toggleVisibility() {
this.props.wrapperElement.classList.toggle(CSS.VISIBLE);
this.props.wrapperElement.classList.toggle(CSS.RMX_VISIBLE);
Copy link
Member

Choose a reason for hiding this comment

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

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 />;
  }

Copy link
Member Author

Choose a reason for hiding this comment

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

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

}

/**
* Updates the state to new route.
* @param {Event} event The route to update to.
*/
updateRoute(event: Event) {
const currentRoute = this.state.route;
this.setState({
route: currentRoute === Route.Variables
? Route.Options
: Route.Variables
});
}

/**
* Renders the component via React.
* @override
*/
/** @override */
render() {
const CurrentRoute = this.state.route === Route.Options ? Options : Overlay;
return (
<div className="rmx-card-wide mdl-card mdl-shadow--6dp">
<div className="mdl-card mdl-shadow--6dp">
Copy link
Member

Choose a reason for hiding this comment

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

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

<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">
{/*}<CurrentRoute variables={this.state.variables} />*/}
<Overlay variables={this.state.variables} />
</div>
</div>
Expand All @@ -167,5 +107,5 @@ export class DOMController extends React.Component<ControllerProps, ControllerSt
/**
* Renders the DOMController component to the overlay wrapper element.
*/
let element = document.getElementById(CONST.ID_OVERLAY_WRAPPER);
let element = document.getElementById(CSS.RMX_OVERLAY_WRAPPER);
ReactDOM.render(<div><DOMController wrapperElement={element} /></div>, element);
86 changes: 0 additions & 86 deletions src/ui/controls/ColorSwatchComponent.tsx
View file
Open in desktop

This file was deleted.

Loading