diff --git a/ghdocs/BESTPRACTICES.md b/ghdocs/BESTPRACTICES.md index 130a90ef9c8365..c04dd9aeffdcdc 100644 --- a/ghdocs/BESTPRACTICES.md +++ b/ghdocs/BESTPRACTICES.md @@ -23,6 +23,138 @@ interface IButton { } ``` +## Use @autobind. Avoid ALL binds in templates + +The autobind decorator simplifies making event callbacks "bound" to the instance. It's easy to use: + +```typescript +class Foo { + + @autobind + public _onClick(ev: React.MouseEvent) { + } + +} +``` + +When we use bind in a template, it means that the function is recreated every time, which is an anti-pattern. + +## For gathering refs, avoid string refs, use resolve functions + +Facebook has deprecated the use of string refs. Plus, string refs don't play very well with some scenarios like in Layered content, where the content may be asynchronously rendered. The recommendation is to use functions to resolve refs. These functions must be pre-bound to + +Bad: +```typescript +public render() { + return
+} +``` + +Bad: +```typescript +public render() { + return
this._root = el } /> +} +``` + +Good: +```typescript +private _root: HTMLElement; +private _resolveRoot: (element: HTMLElement) => any; + +constructor() { + this._resolveRoot = (el: HTMLElement) => this._root = el; +} + +public render() { + return
+} +``` + +Note that it's very critical you do NOT inline the functions in render! This causes weird side effects, because the function +is recreated. If you do this, react will call your function with null to clear it, and then render, and then once complete, will +call it again with the element. This creates timing issues where your ref may be null when you didn't expect. + +See example here that illustates refs being re-evaluated: + +http://codepen.io/dzearing/pen/WGZOaR + +## Use unique keys for items in a mapped array, avoid indexes for keys + +```typescript +public render() { + return ( +
+ { this.props.items.map((item, index)=> ( +
+ +
+ )) } +
+ ); +} +``` + +When you need to map data, require that your data have unique ids available. When you map the data to content, use the id as +the item's key. This helps React's dom diffing to understand which elements represent which items. + +When your code uses index as the key, this can cause unexpected side effects. For example, say that each component you map items to +maintains some internal state, like the value in an uncontrolled input box. Removing an item in the array will not reset that state +and react will think the first text box still represents key "0", which is actually now a different item, and thus will not re-render +the component, leaving the old state intact. + +So lesson is, use actual unique ids for items that come from the data, rather than generated by the ux component, and thus require +that your data has ids so that you can use that as a unique identifier for rendering. + +## Avoid nesting content in an array, wrap mapped content in component instead. + +When we do inline mapped entries, we want to inline the content in the map: + +Bad: +```typescript +class Foo extends React.Component { + public render() { + return ( +
+ { items.map(item => ( +
{ item.name }
+ ))} +
+ ); + } +} +``` + +Better: +```typescript +class Foo extends React.Component { + public render() { + return ( +
+ { items.map(item => ( + + )) } +
+ ); + } +} + +class ItemComponent extends React.Component<...> { + let { item } = this.props; + + render() { + return ( +
{ item.name }
+ ); + } + + @autobind + _onClick(ev: MouseEvent) { + + } +} +``` + ## Extend from BaseComponent instead of React.Component in most cases. In the common folder, there exists a BaseComponent class. For simple components, it may be unnecessary to use. @@ -56,7 +188,208 @@ If specific component elements need special classnames injected, add more classN A component's SCSS file should ONLY include files applicable to the component, and should not define styles for any other component. -# Class name guidelines +# Data/state management best practices + +## Things to flat out avoid + +Singletons and globals. Do not share state of a component via a singleton instance object. This is a pure anti-pattern for the following reasons: + +* Singletons have no lifetime and therefore can't initialize/clean up when they aren't used. +* Singletons often paint you into a corner when you want more than one of the same thing on the page. +* They are often difficult to test without polluting states required for other tests. +* The make non-browser scenario reuse really difficult. (Build tooling, server side code.) + +Data and state should always follow the lifetime of the component that owns it. When that component is no longer needed, the state should +be garbage collected, event handlers should be removed, and xhr requires spawned in that context should be cancelled. + +There are cases where everything should share a common instance of a store; Persona presence is a great example. We have solutions that enable the code within a component scope to access shared stores. See store usage section below. + +## Use const values global to a file, rather than as static members + +Don't use public/private statics, use file scope. + +Bad: +```typescript +class Foo { + private static DEFAULT_DELAY = 300; +} +``` + +Good: +```typescript +const DEFAULT_DELAY = 300; + +class Foo { +} +``` + +Note: using file scopes minimizes the name down to 1 character, whereas this.REALLY_LONG_NAME doesn't minify the name. Caveat being +that if you export it, minify won't touch exports. + +## Use private members only for managing state local to a component that should not affect rendering when mutating + +Bad: +```typescript +class Foo extends React.Component { + + constructor(props) { + super(props); + this.state = { + isMounted: false + }; + } + + componentDidMount() { + this.setState({ + isMounted: true + }); + } + + render() { + return
Hello!
+ } +} +``` + +Good: +```typescript +class Foo extends React.Component { + private _isMounted: boolean; + + constructor(props) { + super(props); + this._isMounted = false; + } + + componentDidMount() { + this._isMounted = true; + } + + render() { + return
Hello!
+ } +} +``` + +Basically privates should only be use for tracking this that in no way affect the frequency of render. In some cases, you many +have an object which fires change events yet doesn't mutate, and you want to re-render on change events. In these cases, privates +may be used to store the object reference. + +## Use component state only for managing local state that is private to a component that should trigger re-render on mutation + +Bad: +```typescript +class Foo extends React.Component { + private _fooRocks: boolean; + + render() { + return
{ `Foo ${ this._fooRocks ? 'rocks' : 'rolls' }
; + } + + private _onClick() { + this._fooRocks = !this._fooRocks; + this.forceUpdate(); + } +``` + +Good: +```typescript +class Foo extends React.Component { + constructor() { + super(props); + + this.state = { + fooRocks: false + }; + } + + render() { + return
{ `Foo ${ this.state.fooRocks ? 'rocks' : 'rolls' }
; + } + + private _onClick() { + this.setState({ + fooRocks: !this.state.fooRocks + }); + } +``` + +## Experimental: Use a store for storing shared state within a component heirachy which must be shared across objects + +While React state management is very useful for simple, private state, it becomes unweildy when many things share that state +and you start ending up with many owners of the same state value. In these cases it may drive you towards a flux solution, but +before we jump there, lets call out a few things. + +* Global singletons are code smell, do not use globals or singletons except for very rare cases +* Do not store complex shared state directly in the components, this is also code smell + +Instead, make a store and wire dumb components together the store. + +What is a store? + +Think of a store as a self aware observable; it maintains state, and provides read/write access to that state. When its state +changes, it can emit change events causing components to re-evaluate. + +Here's an example store which tracks selection: + +```typescript +class SimpleSelectionStore extends BaseStore { + private _selectedKey: string; + + public isSelected(key: string) { + return key === this._selectedKey; + } + + public setSelected(key: string) { + if (this._selectedKey !== key) { + this._selectedKey = key; + this.emitChange(); + } + } +} +``` + +While we do not want globals, we do want simplified access to the stores available in a given component scope. + +You can use the StoreHost component to host stores within any given scope: + +```typescript +const stores = { + selection: new SimpleSelectionStore() +}; + +public render() { + return ( + + + + ); +} +``` + +You can write dumb components: + +```typescript +const Item = (props) => ( +
{ `I am item ${ props.name } and I'm ${ props.isSelected ? 'selected' : 'unselected' }
+); +``` + +And you can use the `connect` function to create a ConnectedItem which wraps the dumb component and translates the props +and stores in its context into props for the dumb component: + +```typescript +export ConnectedItem = connect(Item, (stores, props) => ({ + name: props.item.name, + isSelected: stores.selection.isSelected(props.item.key), + onClick: () => stores.selection.setSelected(props.item.key) +})); +``` + +Now in this setup, your state is in one place, it is composable and can have lifetime, and your dumb ui has no understanding +of the contextual stores provided. + +# CSS class name guidelines TODO: include our class name guidelines. diff --git a/package.json b/package.json index 9c326b3c38f6e9..323879390d4971 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "office-ui-fabric-react", - "version": "0.47.0", + "version": "0.48.0", "description": "Reusable React components for building experiences for Office 365.", "main": "lib/index.js", "typings": "lib/index.d.ts", diff --git a/src/Utilities.ts b/src/Utilities.ts index 9b00e246d1ff05..e95247a58fc45e 100644 --- a/src/Utilities.ts +++ b/src/Utilities.ts @@ -2,3 +2,4 @@ export * from './common/BaseComponent'; export * from './utilities/css'; export * from './utilities/rtl'; export * from './utilities/hoist'; +export * from './utilities/autobind'; diff --git a/src/common/BaseComponent.test.ts b/src/common/BaseComponent.test.tsx similarity index 74% rename from src/common/BaseComponent.test.ts rename to src/common/BaseComponent.test.tsx index 91955bcbc6b384..7c647990d2a7a5 100644 --- a/src/common/BaseComponent.test.ts +++ b/src/common/BaseComponent.test.tsx @@ -1,3 +1,11 @@ +/* tslint:disable:no-unused-variable */ +import * as React from 'react'; +/* tslint:enable:no-unused-variable */ + +import * as ReactTestUtils from 'react-addons-test-utils'; + +let { expect } = chai; + import { BaseComponent } from './BaseComponent'; let { assert } = chai; @@ -54,6 +62,22 @@ describe('BaseComponent', () => { _buildTestFor('render'); _buildTestFor('componentDidUpdate'); _buildTestFor('componentWillUnmount'); + + it('can resolve refs', () => { + class Foo extends BaseComponent<{}, {}> { + public root: HTMLElement; + + public render() { + return
; + } + } + + let component = ReactTestUtils.renderIntoDocument( + + ) as any; + + expect(component.root).to.exist; + }); }); function _buildTestFor(methodName) { diff --git a/src/common/BaseComponent.ts b/src/common/BaseComponent.ts index 3fc781cfabe490..6e7a5d86c8774d 100644 --- a/src/common/BaseComponent.ts +++ b/src/common/BaseComponent.ts @@ -18,6 +18,7 @@ export class BaseComponent extends React.Component { private __async: Async; private __events: EventGroup; private __disposables: IDisposable[]; + private __resolves: { [ name: string ]: (ref: any) => any }; /** * BaseComponent constructor @@ -103,6 +104,29 @@ export class BaseComponent extends React.Component { return this.__events; } + + /** + * Helper to return a memoized ref resolver function. + * @params refName Name of the member to assign the ref to. + * + * @examples + * class Foo extends BaseComponent<...> { + * private _root: HTMLElement; + * + * public render() { + * return
+ * } + * } + */ + protected _resolveRef(refName: string) { + if (!this.__resolves) { + this.__resolves = {}; + } + if (!this.__resolves[refName]) { + this.__resolves[refName] = (ref) => this[refName] = ref; + } + return this.__resolves[refName]; + } } /** diff --git a/src/components/Breadcrumb/Breadcrumb.tsx b/src/components/Breadcrumb/Breadcrumb.tsx index 6ead7fa7f10d6a..a9b55be54209c8 100644 --- a/src/components/Breadcrumb/Breadcrumb.tsx +++ b/src/components/Breadcrumb/Breadcrumb.tsx @@ -7,6 +7,7 @@ import { DirectionalHint } from '../../common/DirectionalHint'; import { getRTL } from '../../utilities/rtl'; import { getId } from '../../utilities/object'; import { css } from '../../utilities/css'; +import { autobind } from '../../utilities/autobind'; import './Breadcrumb.scss'; export interface IBreadcrumbState { @@ -71,7 +72,7 @@ export class Breadcrumb extends BaseComponent
+ onDismiss={ this._onOverflowDismissed } /> ) : (null) }
); } + @autobind private _onOverflowClicked(ev: MouseEvent) { this.setState({ 'isOverflowOpen' : !this.state.isOverflowOpen, @@ -120,6 +122,7 @@ export class Breadcrumb extends BaseComponent { }; this._events = new EventGroup(this); - - this._onLayerDidMount = this._onLayerDidMount.bind(this); } public componentDidUpdate() { @@ -108,6 +107,7 @@ export class Callout extends React.Component { } } + @autobind private _onLayerDidMount() { // This is added so the callout will dismiss when the window is scrolled // but not when something inside the callout is scrolled. diff --git a/src/components/Checkbox/Checkbox.tsx b/src/components/Checkbox/Checkbox.tsx index 5db76129637643..644702fec5202c 100644 --- a/src/components/Checkbox/Checkbox.tsx +++ b/src/components/Checkbox/Checkbox.tsx @@ -1,37 +1,37 @@ import * as React from 'react'; +import { BaseComponent } from '../../common/BaseComponent'; import { ICheckbox, ICheckboxProps } from './Checkbox.Props'; +import { autobind } from '../../utilities/autobind'; import { css } from '../../utilities/css'; import { getId } from '../../utilities/object'; - import './Checkbox.scss'; -export class Checkbox extends React.Component implements ICheckbox { +export interface ICheckboxState { + /** Is true when the control has focus. */ + isFocused?: boolean; + + /** Is true when Uncontrolled control is checked. */ + isChecked?: boolean; +} + +export class Checkbox extends BaseComponent implements ICheckbox { public static defaultProps: ICheckboxProps = { }; private _id: string; - private _checkBox: HTMLElement; - private _checkBoxInput: HTMLInputElement; - private _checkBoxLabel: HTMLLabelElement; + private _checkBox: HTMLInputElement; constructor(props: ICheckboxProps) { super(props); this._id = getId('checkbox-'); - this._onChange = this._onChange.bind(this); - } - - public componentDidMount() { - this._checkBoxInput.addEventListener('focus', this._onFocus.bind(this), false); - this._checkBoxInput.addEventListener('blur', this._onBlur.bind(this), false); - } - - public componentWillUnmount() { - this._checkBoxInput.removeEventListener('focus', this._onFocus.bind(this)); - this._checkBoxInput.removeEventListener('blur', this._onBlur.bind(this)); + this.state = { + isFocused: false, + isChecked: props.defaultChecked || false + }; } public render() { @@ -44,27 +44,33 @@ export class Checkbox extends React.Component implements ICh label } = this.props; + const { isFocused, isChecked } = this.state; + return (
this._checkBox = c } + className={ css('ms-Checkbox', className, { 'is-inFocus': isFocused }) } > this._checkBoxInput = el } + ref={ this._resolveRef('_checkBox') } id={ this._id } name={ this._id } className='ms-Checkbox-input' type='checkbox' onChange={ this._onChange } + onFocus= { this._onFocus } + onBlur= { this._onBlur } aria-checked={ checked } /> @@ -73,37 +79,36 @@ export class Checkbox extends React.Component implements ICh } public get checked(): boolean { - return this._checkBoxInput ? this._checkBoxInput.checked : false; + return this._checkBox ? this._checkBox.checked : false; } public focus() { - if (this._checkBoxInput) { - this._checkBoxInput.focus(); + if (this._checkBox) { + this._checkBox.focus(); } } - private _onFocus(): void { - this._checkBox.classList.add('is-inFocus'); + @autobind + private _onFocus(ev: React.FocusEvent): void { + this.setState({ isFocused: true }); } - private _onBlur(): void { - this._checkBox.classList.remove('is-inFocus'); + @autobind + private _onBlur(ev: React.FocusEvent): void { + this.setState({ isFocused: false }); } + @autobind private _onChange(ev: React.FormEvent) { const { onChange } = this.props; const isChecked = (ev.target as HTMLInputElement).checked; - if (this.props.checked === undefined && isChecked) { - this._checkBoxLabel.classList.add('is-checked'); - } - - if (this.props.checked === undefined && !isChecked) { - this._checkBoxLabel.classList.remove('is-checked'); - } - if (onChange) { onChange(ev, isChecked); } + + if (this.props.checked === undefined) { + this.setState({ isChecked: isChecked }); + } } } diff --git a/src/components/ChoiceGroup/ChoiceGroup.tsx b/src/components/ChoiceGroup/ChoiceGroup.tsx index 2415719fc81289..0f7a01c475c15e 100644 --- a/src/components/ChoiceGroup/ChoiceGroup.tsx +++ b/src/components/ChoiceGroup/ChoiceGroup.tsx @@ -7,6 +7,9 @@ import './ChoiceGroup.scss'; export interface IChoiceGroupState { keyChecked: string; + + /** Is true when the control has focus. */ + keyFocused?: string; } export class ChoiceGroup extends React.Component { @@ -16,28 +19,18 @@ export class ChoiceGroup extends React.Component { - inputElement.addEventListener('focus', this._onFocus.bind(this), false); - inputElement.addEventListener('blur', this._onBlur.bind(this), false); - }); } public componentWillReceiveProps(newProps: IChoiceGroupProps) { @@ -46,23 +39,14 @@ export class ChoiceGroup extends React.Component { - if (inputElement) { - inputElement.removeEventListener('focus', this._onFocus.bind(this)); - inputElement.removeEventListener('blur', this._onBlur.bind(this)); - } - }); - } - public render() { let { label, options, className, required } = this.props; - let { keyChecked } = this.state; + let { keyChecked, keyFocused } = this.state; const titleClassName = css('ms-Label', className, { 'is-required': required @@ -75,20 +59,22 @@ export class ChoiceGroup extends React.Component this._choiceFieldGroup = c } >
{ this.props.label ? : null }
- { options.map((option, index) => ( + { options.map((option) => (
{ this._choiceFieldElements.push(c); return c; } } + className={ css('ms-ChoiceField', { + 'ms-ChoiceField--image': !!option.imageSrc, + 'is-inFocus': option.key === keyFocused + }) + } > { this._inputElements.push(c); return c; } } + ref={ (c): HTMLInputElement => this._inputElement = c } id={ `${this._id}-${option.key}` } className='ms-ChoiceField-input' type='radio' @@ -96,6 +82,8 @@ export class ChoiceGroup extends React.Component { this._renderField(option) } @@ -107,21 +95,23 @@ export class ChoiceGroup extends React.Component { constructor(props: IColorPickerProps) { super(props); - this._onPickerClick = this._onPickerClick.bind(this); - this._onSVChanged = this._onSVChanged.bind(this); - this._onHChanged = this._onHChanged.bind(this); - this._onAChanged = this._onAChanged.bind(this); - this.state = { color: getColorFromString(props.color) }; @@ -91,20 +87,17 @@ export class ColorPicker extends React.Component { ); } - private _onPickerClick() { - this.setState({ - isOpen: !this.state.isOpen - }); - } - + @autobind private _onSVChanged(s: number, v: number) { this._updateColor(updateSV(this.state.color, s, v)); } + @autobind private _onHChanged(h: number) { this._updateColor(updateH(this.state.color, h)); } + @autobind private _onAChanged(a: number) { this._updateColor(updateA(this.state.color, a)); } diff --git a/src/components/ColorPicker/ColorRectangle.tsx b/src/components/ColorPicker/ColorRectangle.tsx index 64544b1de2940e..3d40c3fad2a6c5 100644 --- a/src/components/ColorPicker/ColorRectangle.tsx +++ b/src/components/ColorPicker/ColorRectangle.tsx @@ -6,6 +6,7 @@ import { getFullColorString } from './colors'; import { assign } from '../../utilities/object'; +import { autobind } from '../../utilities/autobind'; import { EventGroup } from '../../utilities/eventGroup/EventGroup'; let hsv2hex = require('color-functions/lib/hsv2hex'); @@ -42,9 +43,6 @@ export class ColorRectangle extends React.Component, IPosi */ beakWidth?: number; + /** + * Aria label for accessibility for the ContextualMenu. + * If none specified no aria label will be applied to the ContextualMenu. + */ + ariaLabel?: string; + } export interface IContextualMenuItem { diff --git a/src/components/ContextualMenu/ContextualMenu.tsx b/src/components/ContextualMenu/ContextualMenu.tsx index f9f55429327bfa..c53350ae81be81 100644 --- a/src/components/ContextualMenu/ContextualMenu.tsx +++ b/src/components/ContextualMenu/ContextualMenu.tsx @@ -4,6 +4,7 @@ import { DirectionalHint } from '../../common/DirectionalHint'; import { FocusZone, FocusZoneDirection } from '../../FocusZone'; import { KeyCodes } from '../../utilities/KeyCodes'; import { EventGroup } from '../../utilities/eventGroup/EventGroup'; +import { autobind } from '../../utilities/autobind'; import { css } from '../../utilities/css'; import { getRTL } from '../../utilities/rtl'; import { getId } from '../../utilities/object'; @@ -99,14 +100,9 @@ export class ContextualMenu extends React.Component this._focusZone = focusZone } rootProps={ { role: 'menu' } } > -
    +
      { items.map((item, index) => ( // If the item name is equal to '-', a divider will be generated. item.name === '-' ? (
    • + className={ css('ms-ContextualMenu-divider', item.className) }/> ) : (
    • - { this._renderMenuItem(item, index, hasCheckmarks, hasIcons) } + className={ css('ms-ContextualMenu-item', item.className) }> + { this._renderMenuItem(item, index, hasCheckmarks, hasIcons) }
    • ) )) } @@ -221,26 +221,37 @@ export class ContextualMenu extends React.Component { location.href = item.href; } : null, - onKeyDown: item.items && item.items.length ? this._onItemKeyDown.bind(this, item) : null, - onMouseEnter: this._onMouseEnter.bind(this, item), - onMouseLeave: this._onMouseLeave, - onMouseDown: (ev: any) => this._onItemMouseDown(item, ev), - disabled: item.isDisabled, - dataCommandKey: index, - role: 'menuitem', - href: item.href, - 'aria-haspopup': item.items && item.items.length ? true : null, - 'aria-owns': item.key === expandedMenuItemKey ? subMenuId : null }, - this._renderMenuItemChildren(item, index, hasCheckmarks, hasIcons)); + 'button', + { + className: css('ms-ContextualMenu-link', { 'is-expanded': (expandedMenuItemKey === item.key) }), + onClick: item.onClick || (item.items && item.items.length) ? this._onItemClick.bind(this, item) : item.href ? () => { location.href = item.href; } : null, + onKeyDown: item.items && item.items.length ? this._onItemKeyDown.bind(this, item) : null, + onMouseEnter: this._onItemMouseEnter.bind(this, item), + onMouseLeave: this._onMouseLeave, + onMouseDown: (ev: any) => this._onItemMouseDown(item, ev), + disabled: item.isDisabled, + dataCommandKey: index, + role: 'menuitem', + href: item.href, + title: item.title, + 'aria-label': ariaLabel, + 'aria-haspopup': item.items && item.items.length ? true : null, + 'aria-owns': item.key === expandedMenuItemKey ? subMenuId : null + }, + this._renderMenuItemChildren(item, index, hasCheckmarks, hasIcons)); } private _renderMenuItemChildren(item: IContextualMenuItem, index: number, hasCheckmarks: boolean, hasIcons: boolean) { @@ -249,17 +260,17 @@ export class ContextualMenu extends React.Component ) : (null) } {(hasIcons) ? ( - ) : (null)} + ) : (null) } { item.name } {(item.items && item.items.length) ? ( - ) : (null)} + ) : (null) }
); } @@ -270,6 +281,7 @@ export class ContextualMenu extends React.Component this._onSubMenuExpand(item, targetElement), 500); + this._enterTimerId = this._async.setTimeout(() => this._onItemSubMenuExpand(item, targetElement), 500); } else { this._enterTimerId = this._async.setTimeout(() => this._onSubMenuDismiss(ev), 500); } } } + @autobind private _onMouseLeave(ev: React.MouseEvent) { this._async.clearTimeout(this._enterTimerId); } @@ -317,7 +330,7 @@ export class ContextualMenu extends React.Component { public componentWillReceiveProps(newProps: IDropdownProps) { this.setState({ - selectedIndex: this._getSelectedIndex(newProps.options, newProps.selectedKey) + selectedIndex: this._getSelectedIndex(newProps.options, newProps.selectedKey), + isDisabled: newProps.isDisabled }); } diff --git a/src/components/FocusZone/FocusZone.tsx b/src/components/FocusZone/FocusZone.tsx index 1a84daadda021f..e1214127b77f3b 100644 --- a/src/components/FocusZone/FocusZone.tsx +++ b/src/components/FocusZone/FocusZone.tsx @@ -8,6 +8,7 @@ import { EventGroup } from '../../utilities/eventGroup/EventGroup'; import { KeyCodes } from '../../utilities/KeyCodes'; import { getRTL } from '../../utilities/rtl'; import { getId } from '../../utilities/object'; +import { autobind } from '../../utilities/autobind'; import { css } from '../../utilities/css'; import { getNextElement, @@ -60,10 +61,6 @@ export class FocusZone extends React.Component implements I }; this._events = new EventGroup(this); - - this._onKeyDown = this._onKeyDown.bind(this); - this._onFocus = this._onFocus.bind(this); - this._onMouseDown = this._onMouseDown.bind(this); } public componentDidMount() { @@ -100,7 +97,8 @@ export class FocusZone extends React.Component implements I aria-labelledby={ ariaLabelledBy } onMouseDownCapture={ this._onMouseDown } onKeyDown={ this._onKeyDown } - onFocus={ this._onFocus } > + onFocus={ this._onFocus } + > { this.props.children }
); @@ -157,6 +155,7 @@ export class FocusZone extends React.Component implements I return false; } + @autobind private _onFocus(ev: React.FocusEvent) { let { onActiveElementChanged } = this.props; @@ -182,12 +181,13 @@ export class FocusZone extends React.Component implements I /** * Handle global tab presses so that we can patch tabindexes on the fly. */ - private _onKeyDownCapture(ev: React.KeyboardEvent) { + private _onKeyDownCapture(ev: KeyboardEvent) { if (ev.which === KeyCodes.tab) { this._updateTabIndexes(); } } + @autobind private _onMouseDown(ev: React.MouseEvent) { const { disabled } = this.props; @@ -218,6 +218,7 @@ export class FocusZone extends React.Component implements I /** * Handle the keystrokes. */ + @autobind private _onKeyDown(ev: React.KeyboardEvent) { const { direction, disabled, isInnerZoneKeystroke } = this.props; @@ -432,7 +433,7 @@ export class FocusZone extends React.Component implements I let distance = -1; if ((targetTop === -1 && targetRect.bottom <= activeRect.top) || - (targetRect.top === targetTop)) { + (targetRect.top === targetTop)) { targetTop = targetRect.top; if (leftAlignment >= targetRect.left && leftAlignment <= (targetRect.left + targetRect.width)) { distance = 0; diff --git a/src/components/GroupedList/GroupFooter.tsx b/src/components/GroupedList/GroupFooter.tsx index 14691145fee7d4..15db445c998140 100644 --- a/src/components/GroupedList/GroupFooter.tsx +++ b/src/components/GroupedList/GroupFooter.tsx @@ -5,6 +5,7 @@ import { IGroup, } from './GroupedList.Props'; import { GroupSpacer } from './GroupSpacer'; +import { autobind } from '../../utilities/autobind'; import './GroupFooter.scss'; export interface IGroupFooter { @@ -15,11 +16,6 @@ export interface IGroupFooter { } export class GroupFooter extends React.Component { - constructor(props: IGroupFooter) { - super(props); - - this._onToggleSummarize = this._onToggleSummarize.bind(this); - } public render() { let { group, groupLevel, footerProps } = this.props; @@ -36,6 +32,7 @@ export class GroupFooter extends React.Component { ); } + @autobind private _onToggleSummarize() { let { group, footerProps } = this.props; let onToggleSummarize = footerProps && footerProps.onToggleSummarize; diff --git a/src/components/GroupedList/GroupHeader.tsx b/src/components/GroupedList/GroupHeader.tsx index f5745ae5a1ec92..39a7bce3874d8c 100644 --- a/src/components/GroupedList/GroupHeader.tsx +++ b/src/components/GroupedList/GroupHeader.tsx @@ -9,6 +9,7 @@ import { GroupSpacer } from './GroupSpacer'; import { Spinner } from '../../Spinner'; import { FocusZone, FocusZoneDirection } from '../../FocusZone'; import { css } from '../../utilities/css'; +import { autobind } from '../../utilities/autobind'; import { IViewport } from '../../utilities/decorators/withViewport'; import './GroupHeader.scss'; @@ -30,10 +31,6 @@ export class GroupHeader extends React.Component { - constructor(props: ILinkProps) { - super(props); - - this._onClick = this._onClick.bind(this); - this._popupWindow = this._popupWindow.bind(this); - } - public render() { let { children, className, href } = this.props; @@ -42,6 +36,7 @@ export class Link extends React.Component { )); } + @autobind private _onClick(ev: React.MouseEvent) { let { popupWindowProps, onClick } = this.props; diff --git a/src/components/MarqueeSelection/MarqueeSelection.tsx b/src/components/MarqueeSelection/MarqueeSelection.tsx index f38904fe560939..8bcf94e40faa12 100644 --- a/src/components/MarqueeSelection/MarqueeSelection.tsx +++ b/src/components/MarqueeSelection/MarqueeSelection.tsx @@ -7,6 +7,7 @@ import { IRectangle } from '../../common/IRectangle'; import { css } from '../../utilities/css'; import { findScrollableParent } from '../../utilities/scrollUtilities'; import { getDistanceBetweenPoints } from '../../utilities/math'; +import { autobind } from '../../utilities/autobind'; import './MarqueeSelection.scss'; @@ -52,8 +53,6 @@ export class MarqueeSelection extends BaseComponent { }; public render() { - let { className, size, imageUrl, imageInitials, initialsColor, presence, primaryText, secondaryText, tertiaryText, optionalText, hidePersonaDetails } = this.props; + let { + className, + size, + imageUrl, + imageInitials, + initialsColor, + presence, + primaryText, + secondaryText, + tertiaryText, + optionalText, + hidePersonaDetails + } = this.props; let presenceElement = null; if (presence !== PersonaPresence.none) { @@ -56,18 +68,17 @@ export class Persona extends React.Component { ) : (null) }
{ tertiaryText }
{ optionalText }
+ { this.props.children }
; return ( -
+
{ size !== PersonaSize.tiny && (
{ imageInitials }
) } - { presenceElement } - { (!hidePersonaDetails || (size === PersonaSize.tiny)) && personaDetails }
); diff --git a/src/components/Toggle/Toggle.tsx b/src/components/Toggle/Toggle.tsx index a776e8147c7438..ab22ddffe75556 100644 --- a/src/components/Toggle/Toggle.tsx +++ b/src/components/Toggle/Toggle.tsx @@ -3,6 +3,8 @@ import { IToggleProps } from './Toggle.Props'; import { css } from '../../utilities/css'; import { Label } from '../../Label'; import { getId } from '../../utilities/object'; +import { autobind } from '../../utilities/autobind'; + import './Toggle.scss'; export interface IToggleState { @@ -28,7 +30,6 @@ export class Toggle extends React.Component { }; this._id = getId('Toggle'); - this._onClick = this._onClick.bind(this); } /** @@ -91,6 +92,7 @@ export class Toggle extends React.Component { } } + @autobind private _onClick() { let { checked, onChanged } = this.props; let { isChecked } = this.state; diff --git a/src/demo/pages/ContextualMenuPage/examples/ContextualMenu.Basic.Example.tsx b/src/demo/pages/ContextualMenuPage/examples/ContextualMenu.Basic.Example.tsx index c44a7b1c0abdf5..3a822688e9e2fe 100644 --- a/src/demo/pages/ContextualMenuPage/examples/ContextualMenu.Basic.Example.tsx +++ b/src/demo/pages/ContextualMenuPage/examples/ContextualMenu.Basic.Example.tsx @@ -14,14 +14,14 @@ export class ContextualMenuBasicExample extends React.Component { public render() { return (
- + { this.state.isContextMenuVisible ? ( { expect(changeCount).equals(10, 'after range selecting from 0 to 2'); }); + it('returns false on isAllSelected when no items are selectable', () => { + let changeEvents = 0; + let selection = new Selection({ + canSelectItem: (item: IObjectWithKey) => false, + onSelectionChanged: () => changeEvents++ + }); + + selection.setItems(setA); + + expect(selection.isAllSelected()).to.equal(false, 'isAllSelected was not false after initialization'); + + selection.setAllSelected(true); + + expect(selection.isAllSelected()).to.equal(false, 'isAllSelected was not false after trying to select all the unselectables'); + + expect(changeEvents).to.equal(0, 'changeEvents were not 0'); + }); + }); \ No newline at end of file diff --git a/src/utilities/selection/Selection.ts b/src/utilities/selection/Selection.ts index 251c35e4b53a87..7857891beb9e9a 100644 --- a/src/utilities/selection/Selection.ts +++ b/src/utilities/selection/Selection.ts @@ -156,10 +156,12 @@ export class Selection implements ISelection { } public isAllSelected(): boolean { + let selectableCount = this._items.length - this._unselectableCount; + return ( (this.count > 0) && (this._isAllSelected && this._exemptedCount === 0) || - (!this._isAllSelected && (this._exemptedCount === this._items.length - this._unselectableCount) && this._items.length > 0)); + (!this._isAllSelected && (this._exemptedCount === selectableCount) && selectableCount > 0)); } public isKeySelected(key: string): boolean { @@ -176,7 +178,9 @@ export class Selection implements ISelection { } public setAllSelected(isAllSelected: boolean) { - if (this._exemptedCount > 0 || isAllSelected !== this._isAllSelected) { + let selectableCount = this._items ? (this._items.length - this._unselectableCount) : 0; + + if (selectableCount > 0 && (this._exemptedCount > 0 || isAllSelected !== this._isAllSelected)) { this._exemptedIndices = {}; this._exemptedCount = 0; this._isAllSelected = isAllSelected; diff --git a/src/utilities/selection/SelectionZone.tsx b/src/utilities/selection/SelectionZone.tsx index 5d94d42c8b955a..c505e1e76931c5 100644 --- a/src/utilities/selection/SelectionZone.tsx +++ b/src/utilities/selection/SelectionZone.tsx @@ -1,5 +1,6 @@ import * as React from 'react'; import { BaseComponent } from '../../common/BaseComponent'; +import { autobind } from '../../utilities/autobind'; import { SelectionLayout } from './SelectionLayout'; import { KeyCodes } from '../KeyCodes'; import { @@ -55,21 +56,6 @@ export class SelectionZone extends BaseComponent { private _isMetaPressed: boolean; private _shouldIgnoreFocus: boolean; - constructor(props: ISelectionZoneProps) { - super(props); - - // Specifically for the click methods, we will want to use React eventing to allow - // React and non React events to stop propagation and avoid the default SelectionZone - // behaviors (like executing onInvoked.) - this._onFocus = this._onFocus.bind(this); - this._onKeyDown = this._onKeyDown.bind(this); - this._onClick = this._onClick.bind(this); - this._onMouseDown = this._onMouseDown.bind(this); - this._onDoubleClick = this._onDoubleClick.bind(this); - this._updateModifiers = this._updateModifiers.bind(this); - this.ignoreNextFocus = this.ignoreNextFocus.bind(this); - } - public componentDidMount() { // Track the latest modifier keys globally. this._events.on(window, 'keydown keyup', this._updateModifiers); @@ -98,6 +84,7 @@ export class SelectionZone extends BaseComponent { * been called on an element, so we need a flag to store the idea that we will bypass the "next" * focus event that occurs. This method does that. */ + @autobind public ignoreNextFocus() { this._shouldIgnoreFocus = true; } @@ -107,6 +94,7 @@ export class SelectionZone extends BaseComponent { * as long as the focus did not originate from a mouse down/touch event. For those cases, we handle them * specially. */ + @autobind private _onFocus(ev: React.FocusEvent) { let target = ev.target as HTMLElement; let { selection, selectionMode } = this.props; @@ -132,6 +120,7 @@ export class SelectionZone extends BaseComponent { } } + @autobind private _onMouseDown(ev: React.MouseEvent) { this._updateModifiers(ev); @@ -156,6 +145,7 @@ export class SelectionZone extends BaseComponent { } } + @autobind private _onClick(ev: React.MouseEvent) { this._updateModifiers(ev); @@ -193,6 +183,7 @@ export class SelectionZone extends BaseComponent { * In multi selection, if you double click within an item's root (but not within the invoke element or input elements), * we should execute the invoke handler. */ + @autobind private _onDoubleClick(ev: React.MouseEvent) { let target = ev.target as HTMLElement; let { selectionMode, onItemInvoked } = this.props; @@ -218,6 +209,7 @@ export class SelectionZone extends BaseComponent { } } + @autobind private _onKeyDown(ev: React.KeyboardEvent) { this._updateModifiers(ev);