Skip to content

DetailsList: enabling Shimmer. #4347

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ThomasMichon merged 13 commits into from
Apr 2, 2018
Merged

DetailsList: enabling Shimmer. #4347

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c6082ef
Changes DetailsList to enable use of a basic Shimmer when default ren…
Mar 23, 2018
cca387b
Change log commit.
Mar 23, 2018
1886c82
Comments out the use of DetailsList with the changes because breaks t…
Mar 23, 2018
3c20c8f
Adds border bottom to shimmer rows when in default state.
Mar 23, 2018
600bf0d
Adds support for RTL layout plus small tweak in logic in DetailsRowFi…
Mar 23, 2018
22ae68f
Rename isShimmer to shimmer. Move _initialRenderItems to constructor.…
Mar 27, 2018
03bca54
Adds type to rowProps constant.
Mar 27, 2018
a8e50a9
Merge branch 'master' into v-vibr/DetailsList-Shimmer
Mar 28, 2018
e282601
Moved some styles to vars.
Mar 28, 2018
735c5f4
Moves the shimmerInitialItems to a constant.
Mar 28, 2018
8179102
Change the language for documentation of enableShimmer.
Mar 28, 2018
e0560bf
Add comment to example.
Mar 28, 2018
1817383
Merge branch 'master' into v-vibr/DetailsList-Shimmer
Vitalius1 Mar 28, 2018
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
11 changes: 11 additions & 0 deletions common/changes/@uifabric/experiments/v-vibr-DetailsList-Shimmer_2018-03-23-00-09.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "@uifabric/experiments",
"comment": "Sets up an example of Shimmer used with DetailsList Component.",
"type": "minor"
}
],
"packageName": "@uifabric/experiments",
"email": "v-vibr@microsoft.com"
}
11 changes: 11 additions & 0 deletions common/changes/office-ui-fabric-react/v-vibr-DetailsList-Shimmer_2018-03-23-00-09.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "Brings changes to DetailsList, DetailsRow and DetailsRowFields to enable use of a basic Shimmer.",
"type": "minor"
}
],
"packageName": "office-ui-fabric-react",
"email": "v-vibr@microsoft.com"
}
2 changes: 1 addition & 1 deletion packages/experiments/src/components/Shimmer/ShimmerPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ export class ShimmerPage extends React.Component<IComponentDemoPageProps, {}> {
<ShimmerLoadDataExample />
</ExampleCard>
<ExampleCard
title='Details List with 1000 items loading in async way.'
title='Details List with 500 items loading in async way and having enabled Shimmer.'
code={ ShimmerApplicationExampleCode }
>
<ShimmerApplicationExample />
Expand Down
174 changes: 130 additions & 44 deletions packages/experiments/src/components/Shimmer/examples/Shimmer.Application.Example.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,23 @@ import {
IExpandingCardProps
} from 'office-ui-fabric-react/lib/HoverCard';
import { createListItems } from '@uifabric/example-app-base';
import './Shimmer.Example.scss';
// import {

@cschleiden cschleiden Mar 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

undo this

@Vitalius1 Vitalius1 Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cschleiden by undo you mean remove the commented import?

@cschleiden cschleiden Mar 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yep :)

// IColumn,
// DetailsList,
// buildColumns,
// SelectionMode,
// DetailsRow,
// IDetailsRowProps
// } from '../../../../../office-ui-fabric-react/src/components/DetailsList';
import {
Shimmer,
} from 'experiments/lib/Shimmer';
import { IColumn, DetailsList, buildColumns } from 'office-ui-fabric-react';

const PAGING_DELAY = 3000;
const ITEMS_COUNT = 1000;
const PAGING_SIZE = 10;
IColumn,
DetailsList,
buildColumns,
SelectionMode,
} from 'office-ui-fabric-react';
import { Toggle } from 'office-ui-fabric-react';

@atneik atneik Mar 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we just add Toggle in the above import?

@Vitalius1 Vitalius1 Mar 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This import was used for the example page when I was importing DetailsList using a relative path. When the changes will be merged, this example page will be revised and I will make sure all imports are correct.

import { Shimmer } from 'experiments/lib/Shimmer';
import './Shimmer.Example.scss';

export interface IItem {
[index: string]: string | number;
Expand All @@ -30,13 +38,6 @@ export interface IItem {
height: number;
}

export interface IShimmerElem {
[index: string]: HTMLElement;
}

// tslint:disable-next-line:no-any
let _items: any[];

const fileIcons: { name: string; }[] = [
{ 'name': 'accdb' },
{ 'name': 'csv' },
Expand All @@ -63,9 +64,19 @@ const fileIcons: { name: string; }[] = [
{ 'name': 'xsn' }
];

const ITEMS_COUNT = 500;
const ITEMS_BATCH_SIZE = 10;
const PAGING_DELAY = 2500;

// tslint:disable-next-line:no-any
let _items: any[];

export interface IShimmerApplicationExampleState {
items?: IItem[];
columns?: IColumn[];
isDataLoaded?: boolean;
isModalSelection?: boolean;
isCompactMode?: boolean;
}

export class ShimmerApplicationExample extends BaseComponent<{}, IShimmerApplicationExampleState> {
Expand All @@ -74,64 +85,138 @@ export class ShimmerApplicationExample extends BaseComponent<{}, IShimmerApplica
constructor(props: {}) {
super(props);

if (!_items) {
_items = createListItems(ITEMS_COUNT);
_items.map((item: IItem) => {
const randomFileType = this._randomFileIcon();
item.thumbnail = randomFileType.url;
});
}

this.state = {
items: _items.slice(0, PAGING_SIZE).concat(new Array(ITEMS_COUNT - PAGING_SIZE)),
columns: _buildColumns()
items: new Array(),
columns: _buildColumns(),
isDataLoaded: false,
isModalSelection: false,
isCompactMode: false
};
}

public render(): JSX.Element {
const { items, columns } = this.state;
const {
items,
columns,
isDataLoaded,
isModalSelection,
isCompactMode
} = this.state;

return (
<div className='shimmerExample-application'>
<p> Hover over location of a row item to see the card </p>
<DetailsList
setKey='items'
items={ items! }
columns={ columns }
onRenderItemColumn={ this._onRenderItemColumn }
onRenderMissingItem={ this._onRenderMissingItem }
/>
<div>
<div className='shimmerExample-toggleButtons'>
<Toggle
label='Enable Modal Selection'
checked={ isModalSelection }
onChanged={ this._onChangeModalSelection }
onText='Modal'
offText='Normal'
/>
<Toggle
label='Enable Compact Mode'
checked={ isCompactMode }
onChanged={ this._onChangeCompactMode }
onText='Compact'
offText='Normal'
/>
<p>Toggle the Load data switch to start async simulation.</p>
<Toggle
label='Load data switch'
checked={ isDataLoaded }
onChanged={ this._onLoadData }
onText='Loaded'
offText='Loading...'
/>
</div>
<div className='shimmerExample-application'>
<DetailsList
setKey='items'
items={ items! }
columns={ columns }
compact={ isCompactMode }
selectionMode={ this.state.isModalSelection ? SelectionMode.multiple : SelectionMode.none }
onRenderItemColumn={ this._onRenderItemColumn }
// enableShimmer={ true }
onRenderMissingItem={ this._onRenderMissingItem }
listProps={ { renderedWindowsAhead: 0, renderedWindowsBehind: 0 } }
/>
</div>
</div>
);
}

private _onRenderMissingItem = (index: number): JSX.Element => {
this._onDataMiss(index as number);
private _onRenderMissingItem = (index: number): React.ReactNode => {
const { isDataLoaded } = this.state;
isDataLoaded && this._onDataMiss(index as number);

return (
<Shimmer />
);
}

private _onDataMiss(index: number): void {
index = Math.floor(index / PAGING_SIZE) * PAGING_SIZE;
// private _onRenderMissingItem = (index: number, rowProps: IDetailsRowProps): React.ReactNode => {
// const { isDataLoaded } = this.state;
// isDataLoaded && this._onDataMiss(index as number);

if (!this._isFetchingItems) {
// return this._onRenderBasicShimmer(rowProps);
// }

this._isFetchingItems = true;
// private _onRenderBasicShimmer(rowProps: IDetailsRowProps): JSX.Element {
// return (
// <Shimmer
// isBaseStyle={ true }
// >
// <DetailsRow { ...rowProps } isShimmer={ true } />
// </Shimmer>
// );
// }

private _onDataMiss = (index: number): void => {
index = Math.floor(index / ITEMS_BATCH_SIZE) * ITEMS_BATCH_SIZE;
if (!this._isFetchingItems) {
this._isFetchingItems = true;
setTimeout(() => {
this._isFetchingItems = false;
// tslint:disable-next-line:no-any
const itemsCopy = ([] as any[]).concat(this.state.items);
itemsCopy.splice.apply(itemsCopy, [index, PAGING_SIZE].concat(_items.slice(index, index + PAGING_SIZE)));

itemsCopy.splice.apply(itemsCopy, [index, ITEMS_BATCH_SIZE].concat(_items.slice(index, index + ITEMS_BATCH_SIZE)));
this.setState({
items: itemsCopy
});
}, PAGING_DELAY);
}
}

private _onLoadData = (checked: boolean): void => {
if (!_items) {
_items = createListItems(ITEMS_COUNT);
_items.map((item: IItem) => {
const randomFileType = this._randomFileIcon();
item.thumbnail = randomFileType.url;
});
}
let { isDataLoaded, items } = this.state;

@atneik atneik Mar 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Whats the point in getting isDataLoaded and items state if we are just going to re-assign them

@Vitalius1 Vitalius1 Mar 23, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That one is used for simulating asynchronous loading which is triggered by the toggle switch in this particular example. So basically it is used for the toggle switch.

isDataLoaded = checked;
if (isDataLoaded) {
items = _items.slice(0, ITEMS_BATCH_SIZE).concat(new Array(ITEMS_COUNT - ITEMS_BATCH_SIZE));
} else {
items = new Array();
}
this.setState({
isDataLoaded: isDataLoaded,
items: items
});
}

private _onChangeModalSelection = (checked: boolean): void => {
this.setState({ isModalSelection: checked });
}

private _onChangeCompactMode = (checked: boolean): void => {
this.setState({ isCompactMode: checked });
}

private _onRenderItemColumn = (item: IItem, index: number, column: IColumn): JSX.Element | string | number => {
const expandingCardProps: IExpandingCardProps = {
onRenderCompactCard: this._onRenderCompactCard,
Expand Down Expand Up @@ -194,7 +279,8 @@ export class ShimmerApplicationExample extends BaseComponent<{}, IShimmerApplica
}

function _buildColumns(): IColumn[] {
const columns: IColumn[] = buildColumns(_items);
const _item = createListItems(1);
const columns: IColumn[] = buildColumns(_item);

columns.forEach((column: IColumn) => {
if (column.key === 'thumbnail') {
Expand Down
4 changes: 0 additions & 4 deletions packages/experiments/src/components/Shimmer/examples/Shimmer.Example.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,4 @@
text-decoration: underline;
cursor: pointer;
}

.shimmerExample-application .ms-Shimmer-container{
margin-left: 42px;
}
}
37 changes: 24 additions & 13 deletions packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,17 @@ const ISPADDED_WIDTH = 24;
const DEFAULT_RENDERED_WINDOWS_AHEAD = 2;
const DEFAULT_RENDERED_WINDOWS_BEHIND = 2;

const SHIMMER_INITIAL_ITEMS = 10;

@withViewport
export class DetailsList extends BaseComponent<IDetailsListProps, IDetailsListState> implements IDetailsList {
public static defaultProps = {
layoutMode: DetailsListLayoutMode.justified,
selectionMode: SelectionMode.multiple,
constrainMode: ConstrainMode.horizontalConstrained,
checkboxVisibility: CheckboxVisibility.onHover,
isHeaderVisible: true
isHeaderVisible: true,
enableShimmer: false
};

// References
Expand All @@ -83,6 +86,7 @@ export class DetailsList extends BaseComponent<IDetailsListProps, IDetailsListSt
private _dragDropHelper: DragDropHelper | null;
private _initialFocusedIndex: number | undefined;
private _pendingForceUpdate: boolean;
private _shimmerInitialItems: any[];

private _columnOverrides: {
[key: string]: IColumn;
Expand Down Expand Up @@ -269,7 +273,8 @@ export class DetailsList extends BaseComponent<IDetailsListProps, IDetailsListSt
getKey,
listProps,
usePageCache,
onShouldVirtualize
onShouldVirtualize,
enableShimmer
} = this.props;
const {
adjustedColumns,
Expand Down Expand Up @@ -312,6 +317,10 @@ export class DetailsList extends BaseComponent<IDetailsListProps, IDetailsListSt

const rowCount = (isHeaderVisible ? 1 : 0) + GetGroupCount(groups) + (items ? items.length : 0);

if (enableShimmer) {
this._shimmerInitialItems = new Array(SHIMMER_INITIAL_ITEMS);
}

@cschleiden cschleiden Mar 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this is done for every render call?

@Vitalius1 Vitalius1 Mar 26, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved it in the constructor. Good catch... It was called on every render )


return (
// If shouldApplyApplicationRole is true, role application will be applied to make arrow keys work
// with JAWS.
Expand Down Expand Up @@ -398,7 +407,7 @@ export class DetailsList extends BaseComponent<IDetailsListProps, IDetailsListSt
<List
ref={ this._list }
role='presentation'
items={ items }
items={ enableShimmer && !items.length ? this._shimmerInitialItems : items }

@cschleiden cschleiden Mar 28, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What happens here when the items array is actually just []? Let's say I make an async call, while it's doing that I want to display the shimmer, but then I just get 0 results and want to display an empty list? Will I have to modify the enableShimmer prop in that case (make it dependent on the loading status) or can we come up with a better interface?

Or, as a cheap fix, don't check for empty length but null?

@Vitalius1 Vitalius1 Mar 28, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@cschleiden From my testing with DetailsList it seems that it always needs at least an empty array in order for it to mount. If you provide a null or undefined to items it breaks during the constructor phase and more specific when creating a new Selection on line 123. So checking for null is not an option cause it will never be null. Correct me if I'm wrong...

@ThomasMichon ThomasMichon Mar 28, 2018

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.

My suggestion to @Vitalius1 was that to indicate that [] is legitimate, you'll need to set enableShimmer={ false } if you pass items={ [] }.

@Vitalius1 Vitalius1 Mar 28, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If you think about it shimmer should be dependent on loading status. After all it's a placeholder while waiting for data. The enableShimmer right now all is doing is creating an array of 10 null values so that the onRenderMissingItem we provide would render 10 shimmer lines. That number it's an average was discussed with designers as we don't know how many items are coming.

onRenderCell={ this._onRenderListCell(0) }
usePageCache={ usePageCache }
onShouldVirtualize={ onShouldVirtualize }
Expand Down Expand Up @@ -458,15 +467,7 @@ export class DetailsList extends BaseComponent<IDetailsListProps, IDetailsListSt
adjustedColumns: columns
} = this.state;

if (!item) {
if (onRenderMissingItem) {
return onRenderMissingItem(index);
}

return null;
}

return onRenderRow({
const rowProps = {
item: item,

@ThomasMichon ThomasMichon Mar 26, 2018

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.

rowProps: IDetailsRowProps
to get type enforcement.

itemIndex: index,
compact: compact,
Expand All @@ -487,7 +488,17 @@ export class DetailsList extends BaseComponent<IDetailsListProps, IDetailsListSt
getRowAriaDescribedBy: getRowAriaDescribedBy,
checkButtonAriaLabel: checkButtonAriaLabel,
checkboxCellClassName: checkboxCellClassName,
}, this._onRenderRow);
};

if (!item) {
if (onRenderMissingItem) {
return onRenderMissingItem(index, rowProps);
}

return null;
}

return onRenderRow(rowProps, this._onRenderRow);
}

private _onGroupExpandStateChanged(isSomeGroupExpanded: boolean) {
Expand Down
10 changes: 9 additions & 1 deletion packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,15 @@ export interface IDetailsListProps extends React.Props<DetailsList>, IWithViewpo
dragDropEvents?: IDragDropEvents;

/** Callback for what to render when the item is missing. */
onRenderMissingItem?: (index?: number) => React.ReactNode;
onRenderMissingItem?: (index?: number, rowProps?: IDetailsRowProps) => React.ReactNode;

/**
* If set to true and we provide an empty array to DetailsList while waiting the API call, it will render 10 shimmer lines.
* When data comes back check if the array of items from the data source is empty.

@cschleiden cschleiden Mar 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm not sure why the details list should care about any API calls

@Vitalius1 Vitalius1 Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed the language as I was thinking more in terms of Items-scope in odsp-common when wrote this explanation.

@cschleiden cschleiden Mar 27, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks!

* If so, we need to provide a false value to this prop to prevent continuos shimmer animation in case you acces an empty folder.

@atneik atneik Mar 23, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: typo
Also not sure if we need to mention how it should be used in a particular case.

@Vitalius1 Vitalius1 Mar 23, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well I just wanted to make sure users know what would happen if the folder they open is empty when the API call is back but the 10 lines of shimmer will infinitely run. What would you suggest?

@cschleiden cschleiden Mar 26, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

spelling

@Vitalius1 Vitalius1 Mar 27, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks...

* @default false
*/
enableShimmer?: boolean;

/**
* An override to render the details header.
Expand Down
Loading