Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
24dd626
Adding autoscroll utility and marquee selection.
dzearing Aug 1, 2016
3921752
More fixes.
dzearing Aug 3, 2016
d5ec54c
Merge pull request #1 from OfficeDev/master
dzearing Aug 3, 2016
58281d9
Enables selection preservation even when items dematerialize.
dzearing Aug 3, 2016
c4ced04
Math rounding tweak in auto scrolling.
dzearing Aug 3, 2016
2e0a119
Updating small nits.
dzearing Aug 3, 2016
ae2bd96
Merge pull request #2 from OfficeDev/master
dzearing Aug 4, 2016
1272674
Merge branch 'master' of https://github.com/dzearing/office-ui-fabric…
dzearing Aug 4, 2016
bb4d1c0
Adding example page, improving props documentation, adding memoizatio…
dzearing Aug 4, 2016
089fd22
More performance improvements.
dzearing Aug 5, 2016
456aee3
Moving files to a more logical location.
dzearing Aug 5, 2016
3c3b43d
Missing an index change.
dzearing Aug 5, 2016
66ef16d
Adding more best practices content.
dzearing Aug 5, 2016
6729ebd
Updating documentation.
dzearing Aug 5, 2016
9d12f13
Removing unnecessary call.
dzearing Aug 5, 2016
3c08105
Removing dir from html.
dzearing Aug 5, 2016
4295470
Removing an unnecessary measure from autoscroll.
dzearing Aug 6, 2016
0c7a8a4
Updating basic details list example to use marquee selection.
dzearing Aug 8, 2016
fb61f66
With scrolltop fix (#3)
dzearing Aug 8, 2016
7ade6e9
Improving the example by removing the images.
dzearing Aug 8, 2016
bd5777d
Removing the scroll monitoring and css tweaking from Fabric component…
dzearing Aug 8, 2016
dca18b5
Fixing issues related to safari support.
dzearing Aug 8, 2016
f329966
Minor improvement to EventGroup.
dzearing Aug 8, 2016
59ab874
Lint fixes.
dzearing Aug 8, 2016
4079b67
Updates for PR comments.
dzearing Aug 8, 2016
4a08c31
Fixing hovers.
dzearing Aug 8, 2016
fa542ac
A few more fixes to test page and styles.
dzearing Aug 8, 2016
7f48aef
Removing lint error.
dzearing Aug 8, 2016
e85baa6
Cleanup.
dzearing Aug 8, 2016
142872f
Adds ability to select from anywhere in the scrollable parent. Also f…
dzearing Aug 9, 2016
b891954
Merge branch 'master' of https://github.com/OfficeDev/office-ui-fabri…
dzearing Aug 9, 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
69 changes: 61 additions & 8 deletions ghdocs/BESTPRACTICES.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,69 @@
# Component design

## Build many smaller components and composite them together.

Often we want to build something complex, like a CommandBar. We see a picture of what we want and we build one giant component that does this. Then we find other scenarios that have overlaps. The CommandBar contains a SearchBox, a set of left side collapsable links and right side links. We may find we have other cases that just want a set of links, without the extra overhead. We may also find cases where we just want a single command bar item with a dropdown menu.

This is an example where instead of building a single large component, we should build more atomic building blocks that are puzzled together. While it may be more effort to think in smaller blocks, it makes the code easier to reuse and often simpler to maintain.

## Use a .Props.ts file to extract out the public contracts that should be supported and documented.

A props file contains all of the interface contracts that the user should know about to use the component. It is the "contract" for the component. When we evaluate semversioning, we look through the changes at Props files to determine if the change is a major, minor, or patch.

The props files are also auto documented. All JSDoc comments will be extracted and shown on the demo site for documentation.

When your component exposes public methods/properties, define an interface for the component in the props file and implement the interface. The auto documentation will interpret the I{Component} interface as the class contract and show it in the documentation as the class definition.

```typescript
interface IButton {
/**
* Sets focus to the button element.
*/
focus(): void;
}
```

## 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.

If you extend this, you get a few useful utilities:

_events: An instance of the EventGroup, scoped to the component. It will auto dispose on component unmounting so that you don't forget.

_async: A collection of utilities for performing async operations, scoped to the component. This includes setTimeout/setInterval helpers as well as utilities for generating throttled/debounced wrappers. Again, anything you use here will be automatically cleaned up on unmounting.

_disposables: An array of IDisposable instances. If you have things you want disposed, you can push them into this.

autoBindCallbacks: A helper method that will automatically bind _on methods, to simplify the manual binding of event callbacks.

Another interesting thing is that when exceptions occur within React's methods, things tend to silently fail. With the BaseComponent, we
make all methods "safe" meaning that if an exception occurs, we send the exception to a global callback which can be hooked up to a telemetry post. By default however, we forward the exception to console.error with the exact class/method that threw the exception so that there is an obvious hint what went wrong.

There are some cases where it may be overkill to subclass from this; a simple Button wrapper for example really doesn't need to be more than a simple stateless component and doesn't need extra imports, which would result in making Button's dependency graph heavier. Use your best judgement.

## Use React eventing, unless you need to use native.

Be aware that React eventing and DOM eventing are two different systems. They do not play well with each other. DOM event handlers will always fire BEFORE React events, regardless of the DOM structure. This can introduce unexpected bugs in code that mixes both React and native DOM eventing.

Unfortunately there are plenty of scenarios where we must mix the two systems together; for example, you may need to listen for application-wide clicks that bubble up to window in order to implement a light-dismiss behavior. Or perhaps you need to listen for window resizes. Or maybe you need to observe scroll events so that you can hide/show something.

We use the EventGroup object for abstracting native eventing. It is simple to use; there is an "on" method and an "off" method that wrap calling addEventListener in modern browsers (or attachEvent in legacy IE.) Again if you're using the BaseComponent, it is already available to you via the _events property.

## Root elements should have a component class name.

Every component's root element should have a ms-Component class name. Additinally the user should be able to provide their own className via prop that should be added to the class list of the root element.

If specific component elements need special classnames injected, add more classNames to props.

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

TODO: include our class name guidelines.

Example:

ms-Component-area--flags

# Style guidelines
Expand Down Expand Up @@ -35,14 +96,6 @@ Additionaly try to have symetrical paddings rather than using padding-right or l

E.g. using ms-font-s classname in a component is forbidden. It makes overriding CSS rules really painful. Instead, use @include ms-font-m;

## Root elements should have a component class name.

Every component's root element should have a ms-Component class name. Additinally the user should be able to provide their own className via prop that should be added to the class list of the root element.

If specific component elements need special classnames injected, add more classNames to props.

A component's SCSS file should ONLY include files applicable to the component, and should not define styles for any other component.

# Example page guidelines

Examples should follow a naming convention: Component.Desc.Example.ts
Expand Down
2 changes: 1 addition & 1 deletion index.html
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!doctype html>
<html dir="ltr">
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
<meta http-equiv="X-UA-Compatible" content="IE=edge" />
Expand Down
3 changes: 3 additions & 0 deletions src/MarqueeSelection.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
export * from './components/MarqueeSelection/MarqueeSelection';
export * from './components/MarqueeSelection/MarqueeSelection.Props';
export * from './utilities/selection/index';
16 changes: 16 additions & 0 deletions src/common/BaseComponent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,11 @@ import { Async } from '../utilities/Async/Async';
import { EventGroup } from '../utilities/eventGroup/EventGroup';
import { IDisposable } from './IDisposable';

// Ensure that the HTML element has a dir specified. This helps to ensure RTL/LTR macros in css for all components will work.
if (document && document.documentElement && !document.documentElement.getAttribute('dir')) {
document.documentElement.setAttribute('dir', 'ltr');
}

export class BaseComponent<P, S> extends React.Component<P, S> {
/**
* External consumers should override BaseComponent.onError to hook into error messages that occur from
Expand Down Expand Up @@ -47,6 +52,17 @@ export class BaseComponent<P, S> extends React.Component<P, S> {
return (results && results.length > 1) ? results[1] : '';
}

/**
* Gives the class constructor, will iterate through prototype methods prefixed with "_on" and bind them to "this".
* Example: in your constructor, you'd have: this.autoBindCallbacks(MyComponent); */
protected autoBindCallbacks(object: Function) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it would make a lot more sense just to define a @bind decorator:

@bind
private _onClick(event: MouseEvent) {
    this.setState({
        isActive: true
    });
}

The @bind decorator would cleanly handle the binding once the instance was constructed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I like that suggestion. I'll remove it here and approach it that way separately.

for (let methodName in object.prototype) {
if (methodName.indexOf('_on') === 0) {
this[methodName] = this[methodName].bind(this);
}
}
}

/** Allows subclasses to push things to this._disposables to be auto disposed. */
protected get _disposables(): IDisposable[] {
if (!this.__disposables) {
Expand Down
15 changes: 5 additions & 10 deletions src/components/DetailsList/DetailsHeader.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
import * as React from 'react';
import { BaseComponent } from '../../common/BaseComponent';
import { IColumn, DetailsListLayoutMode, ColumnActionsMode } from './DetailsList.Props';
import { FocusZone, FocusZoneDirection } from '../../FocusZone';
import { Check } from '../Check/Check';
import { GroupSpacer } from '../GroupedList/GroupSpacer';
import { css } from '../../utilities/css';
import { ISelection, SelectionMode, SELECTION_CHANGE } from '../../utilities/selection/interfaces';
import { getRTL } from '../../utilities/rtl';
import { EventGroup } from '../../utilities/eventGroup/EventGroup';
import './DetailsHeader.scss';

const MOUSEDOWN_PRIMARY_BUTTON = 0; // for mouse down event we are using ev.button property, 0 means left button
Expand Down Expand Up @@ -47,19 +47,15 @@ export interface IColumnResizeDetails {
columnMinWidth: number;
}

export class DetailsHeader extends React.Component<IDetailsHeaderProps, IDetailsHeaderState> {
export class DetailsHeader extends BaseComponent<IDetailsHeaderProps, IDetailsHeaderState> {
public refs: {
[key: string]: React.ReactInstance;
focusZone: FocusZone;
};

private _events: EventGroup;

constructor(props: IDetailsHeaderProps) {
super(props);

this._events = new EventGroup(this);

this.state = {
columnResizeDetails: null,
groupNestingDepth: this.props.groupNestingDepth,
Expand All @@ -76,10 +72,6 @@ export class DetailsHeader extends React.Component<IDetailsHeaderProps, IDetails
this._events.on(selection, SELECTION_CHANGE, this._onSelectionChanged);
}

public componentWillUnmount() {
this._events.dispose();
}

public componentWillReceiveProps(newProps) {
let { groupNestingDepth } = this.state;

Expand Down Expand Up @@ -287,6 +279,9 @@ export class DetailsHeader extends React.Component<IDetailsHeaderProps, IDetails
originX: ev.clientX
}
});

ev.preventDefault();
ev.stopPropagation();
}

private _onSelectionChanged() {
Expand Down
38 changes: 19 additions & 19 deletions src/components/DetailsList/DetailsList.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -303,25 +303,25 @@ export class DetailsList extends React.Component<IDetailsListProps, IDetailsList
}

return (
<DetailsRow
item={ item }
itemIndex={ index }
columns={ columns }
groupNestingDepth={ nestingDepth }
selectionMode={ selectionMode }
selection={ selection }
onDidMount={ this._onRowDidMount }
onWillUnmount={ this._onRowWillUnmount }
onRenderItemColumn={ onRenderItemColumn }
eventsToRegister={ eventsToRegister }
dragDropEvents={ dragDropEvents }
dragDropHelper={ dragDropHelper }
viewport={ viewport }
checkboxVisibility={ checkboxVisibility }
getRowAriaLabel={ getRowAriaLabel }
canSelectItem={ canSelectItem }
checkButtonAriaLabel={ checkButtonAriaLabel }
/>
<DetailsRow
item={ item }
itemIndex={ index }
columns={ columns }
groupNestingDepth={ nestingDepth }
selectionMode={ selectionMode }
selection={ selection }
onDidMount={ this._onRowDidMount }
onWillUnmount={ this._onRowWillUnmount }
onRenderItemColumn={ onRenderItemColumn }
eventsToRegister={ eventsToRegister }
dragDropEvents={ dragDropEvents }
dragDropHelper={ dragDropHelper }
viewport={ viewport }
checkboxVisibility={ checkboxVisibility }
getRowAriaLabel={ getRowAriaLabel }
canSelectItem={ canSelectItem }
checkButtonAriaLabel={ checkButtonAriaLabel }
/>
);
}

Expand Down
28 changes: 28 additions & 0 deletions src/components/MarqueeSelection/MarqueeSelection.Props.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import * as React from 'react';
import { ISelection } from '../../utilities/selection/interfaces';
import { MarqueeSelection } from './MarqueeSelection';

export interface IMarqueeSelectionProps extends React.Props<MarqueeSelection> {
/**
* The selection object to interact with when updating selection changes.
*/
selection: ISelection;

/**
* The base element tag name to render the marquee bounding area within.
* @default div
*/
rootTagName?: string;

/**
* Optional props to mix into the root element.
*/
rootProps?: React.HTMLProps<HTMLDivElement>;

/**
* Optional callback that is called, when the mouse down event occurs, in order to determine
* if we should start a marquee selection. If true is returned, we will cancel the mousedown
* event to prevent upstream mousedown handlers from executing.
*/
onShouldStartSelection?: (ev: React.MouseEvent) => boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this should be widened or altered.
To control whether or not marquee selection is allowed at all, I think passing an isEnabled flag makes more sense. However, to control whether or not a current drag gesture should activate a marquee, I think you should pass the proposed initial selection Rectangle so the owner can decide whether to let selection continue. The MouseEvent by itself is insufficient.

@bind
private _onShouldStartSelection(area: Rectangle, event: React.MouseEvent): boolean {
    if (!this._isDropping) {
        return false;
    }

    if (area.width > 10 || area.height > 10) {
        return true;
    }

    return false;
}

@dzearing dzearing Aug 8, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not sure that passing along the rectangle helps at all. It would force the caller to figure out whats in the box, which pushes some of the logic in more than one place. However, it is possible we could try to identify if the pixel you clicked on is an item and pass that as a parameter. That could make it cleaner.

}
33 changes: 33 additions & 0 deletions src/components/MarqueeSelection/MarqueeSelection.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
@import '../../common/common';

.ms-MarqueeSelection {
position: relative;
cursor: default;
}

.ms-MarqueeSelection-dragMask {
position: absolute;
background: rgba(255, 0, 0, 0);
left: 0;
top: 0;
right: 0;
bottom: 0;
}

.ms-MarqueeSelection-box {
position: absolute;
box-sizing: border-box;
border: 1px solid $ms-color-themePrimary;
pointer-events: none;
}

.ms-MarqueeSelection-boxFill {
position: absolute;
box-sizing: border-box;
background-color: $ms-color-themePrimary;
opacity: .1;
left: 0;
top: 0;
right: 0;
bottom: 0;
}
Loading