DetailsList: enabling Shimmer. by Vitalius1 · Pull Request #4347 · microsoft/fluentui

Vitalius1 · 2018-03-23T00:22:35Z

Pull request checklist

Addresses an existing issue: Fixes #0000
Include a change request file using $ npm run change

Description of changes

Changes DetailsList by giving it a new prop enableShimmer and changes the existing one onRenderMissingItem to enable the use of the ShimmerComponent by leveraging them.
Gives DetailsRow and DetailsRowFields a new prop isShimmer in order to do some logic and enable the styling added to the DetailsRow.scss to recreate the visuals proposed by designers for a basic default shimmer.
Sets up an example in experiments package for Shimmer used within DetilsList

Focus areas to test

Need some feedback on the proposed changes.

…derers are used.

…he build trying to import directly from the path provided.

Vitalius1 · 2018-03-23T02:20:58Z

I had to change the example page for the Shimmer using the changes I am proposing because the build was breaking because of example page trying to import the DetailsList using a relative path as oppose to using import {...} from 'office-ui-fabric-react';

Have commented out the code which shows how we would use the changes to enable use of Shimmer with DetailsList.

Jahnp · 2018-03-23T19:39:58Z

Awesome. Might be good if @aditima could take a look at this as well.

atneik

Looks good! Get an approval from Aditi / Thomas too

atneik · 2018-03-23T18:37:56Z

packages/experiments/src/components/Shimmer/examples/Shimmer.Application.Example.tsx

+  buildColumns,
+  SelectionMode,
+} from 'office-ui-fabric-react';
+import { Toggle } from 'office-ui-fabric-react';


Can we just add Toggle in the above import?

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.

atneik · 2018-03-23T18:45:17Z

packages/experiments/src/components/Shimmer/examples/Shimmer.Application.Example.tsx

+        item.thumbnail = randomFileType.url;
+      });
+    }
+    let { isDataLoaded, items } = this.state;


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

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.

atneik · 2018-03-23T19:00:36Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsRow.scss

+
+// 40px to take into account the checkbox of items if present.
+.shimmerLeftBorder {
+  border-left: 40px solid $detailsList-item-default-background-color;


You might need to use @include border-* mixin here and above so that it works as expected in RTL (right-to-left)

Included the mixin and it works like a charm!

atneik · 2018-03-23T19:01:49Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.types.ts

+  /**
+   * 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.
+   * If so, we need to provide a false value to this prop to prevent continuos shimmer animation in case you acces an empty folder.


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

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?

atneik · 2018-03-23T20:11:42Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsRowFields.tsx


-          cellContent = render ? render(item, itemIndex, column) : this._getCellText(item, column);
+          cellContent = render ?
+            !isShimmer ?


Could do this with a single ternary conditional

…elds.

dzearing · 2018-03-26T22:02:41Z

@ThomasMichon and @aditima FYI

dzearing · 2018-03-26T22:03:29Z

Also @cschleiden

dzearing · 2018-03-26T22:06:35Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsRow.tsx

  checkboxCellClassName?: string;
  rowFieldsAs?: React.StatelessComponent<IDetailsRowFieldsProps> | React.ComponentClass<IDetailsRowFieldsProps>;
  className?: string;
+  isShimmer?: boolean;


can you just call this shimmer?

<DetailsRow shimmer />

Changed from isShimmer to shimmer

cschleiden · 2018-03-26T22:10:18Z

packages/experiments/src/components/Shimmer/examples/Shimmer.Application.Example.tsx

 } from 'office-ui-fabric-react/lib/HoverCard';
 import { createListItems } from '@uifabric/example-app-base';
-import './Shimmer.Example.scss';
+// import {


@cschleiden by undo you mean remove the commented import?

cschleiden · 2018-03-26T22:12:29Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.types.ts

+  /**
+   * 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.
+   * If so, we need to provide a false value to this prop to prevent continuos shimmer animation in case you acces an empty folder.


cschleiden · 2018-03-26T22:12:54Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.types.ts

+
+  /**
+   * 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.


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

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

cschleiden · 2018-03-26T22:14:26Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.tsx


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


this is done for every render call?

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

Vitalius1 · 2018-03-27T00:32:11Z

@cschleiden @aditima @ThomasMichon @dzearing @atneik
When Shimmer Component will be mature enough to be moved from experiments to office-ui-fabric-react package, I was thinking integrating it internally in DetailsList so it could be switched on by simply providing enableShimmer prop without leveraging onRenderMissingItem. Would that raise any concerns of the bundle size?

… Change some language for enableShimmer description.

Vitalius1 · 2018-03-27T18:41:54Z

@dzearing @cschleiden @aditima @ThomasMichon @atneik
Any more changes need to be done? Need this to unblock integration to itemsScope. Will be happy to address any concerns.

cschleiden · 2018-03-28T17:18:11Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.tsx

                      ref={ this._list }
                      role='presentation'
-                      items={ items }
+                      items={ enableShimmer && !items.length ? this._shimmerInitialItems : items }


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?

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

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

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.

cschleiden · 2018-03-28T17:18:45Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsRow.scss

 $compactRowVerticalPadding: 6px;
 $isPaddedMargin: 24px;
+$rowShimmerHorizontalBorder: $rowHorizontalPadding;
+$rowShimmerVerticalBorder: ($rowHeight - 7px) / 2;


Should we have a var for 7px? What does it represent?

This represents the height of the shimmering lines to render. I assume it's logical to be moved to a var.

cschleiden · 2018-03-28T17:20:52Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsRow.tsx

+      />
+    );
+    // Rendering Shimmer Animation outside the focus zone
+    if (shimmer) {


Is the row component the right place for this? Or should we go one level higher and just not render the row and instead render that shimmer component from the detailslist?

We intend to use DetailsRow in the host application so that we can leverage the DetailsRowFields styling to look as we have columns with shimmer lines not just a single line for the entire width of the row. So we need this in here. Eventually I think the shimmer will be moved inside the detailsList so that we are not exposing DetailsRow to the host.

@cschleiden As of now the Shimmer component can not be imported to DetailsList because it's part of experiments package, which is not a dependency of office-ui-fabric-react.
When the shimmer will be mature enough to be moved along side with other Fabric components I intend to use it internally in DetailsList and even then we should check if the bundle size of the DetailsList is going to be something to worry about.

Wait, but shimmer will still be included in the bundle?

@cschleiden Not sure what do you mean? How it will be included if I am not importing it anywhere. As of now it's not included, but in the future we might do that...

You're right my bad. I guess then I don't really understand your approach for including it in detailslist... Why do we have to have that 10 item array there if the rendering is the user's responsibility? I could just do

items={isLoading ? Array(10).fill(null) : realItems}

wherever I use the List? And nothing would have to change

@cschleiden
I think we took this decision for consistency. User should not decide how many lines of shimmer should be rendered in the beginning. DetailsList will always give them 10 lines as we don't know how many realItems will eventually be rendered. I also will add a fading-out to bottom applying a decreasing opacity in styles based on this number (this will remove the expectancy of the user of how many items to wait even if we have 10 lines of shimmer). Without mergeStyle I can not pass a number the user will give me to .sass, so if I know a hardcoded number I could do a loop inside .sass and apply this opacity.

P.S. this number could change as soon as we can demo this to designers.

ThomasMichon · 2018-03-26T22:43:38Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.tsx

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


rowProps: IDetailsRowProps
to get type enforcement.

ThomasMichon · 2018-03-28T19:27:25Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.tsx

      minimumPixelsForDrag: props.minimumPixelsForDrag
    }) : null;
    this._initialFocusedIndex = props.initialFocusedIndex;
+    this._shimmerInitialItems = props.enableShimmer ? new Array(SHIMMER_INITIAL_ITEMS) : [];


Do

const SHIMMER_ITEMS = new Array(SHIMMER_INITIAL_ITEMS);

at the top of the file. Just re-use the const in the render below.

Moved it to constant as requested.

@cschleiden @ThomasMichon
I agree with Thomas the the host application should have the control to switch off the enableShimmer because it's the only one who knows about the loading status. And because Shimmer it's a placeholder for while waiting for that status to change it should be flipped on/off by the user.

As of now we are using the existing prop onRenderMissingItem, but later we intend to move it in DetailsList and all the host application will need to do is to on/off the shimmer

ThomasMichon · 2018-03-28T19:28:15Z

packages/office-ui-fabric-react/src/components/DetailsList/DetailsList.tsx

                      ref={ this._list }
                      role='presentation'
-                      items={ items }
+                      items={ enableShimmer && !items.length ? this._shimmerInitialItems : items }


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

Vitalius1 · 2018-04-02T21:12:11Z

@cschleiden @aditima @ThomasMichon @dzearing @atneik
Could anyone unblock this PR or decide what we do further? It's blocking me to continue with the Shimmer Integration.

Remove blocking review

Vitalie Braga added 3 commits March 22, 2018 17:00

Changes DetailsList to enable use of a basic Shimmer when default ren…

c6082ef

…derers are used.

Change log commit.

cca387b

Comments out the use of DetailsList with the changes because breaks t…

1886c82

…he build trying to import directly from the path provided.

atneik requested review from atneik and dzearing March 23, 2018 03:56

Adds border bottom to shimmer rows when in default state.

3c20c8f

atneik requested a review from ThomasMichon March 23, 2018 19:16

atneik approved these changes Mar 23, 2018

View reviewed changes

atneik requested a review from aditima March 23, 2018 20:13

Adds support for RTL layout plus small tweak in logic in DetailsRowFi…

600bf0d

…elds.

dzearing reviewed Mar 26, 2018

View reviewed changes

cschleiden suggested changes Mar 26, 2018

View reviewed changes

Vitalie Braga added 2 commits March 26, 2018 17:33

Rename isShimmer to shimmer. Move _initialRenderItems to constructor.…

22ae68f

… Change some language for enableShimmer description.

Adds type to rowProps constant.

03bca54

cschleiden previously requested changes Mar 28, 2018

View reviewed changes

Vitalie Braga added 2 commits March 28, 2018 11:41

Merge branch 'master' into v-vibr/DetailsList-Shimmer

a8e50a9

Moved some styles to vars.

e282601

ThomasMichon requested changes Mar 28, 2018

View reviewed changes

Vitalie Braga and others added 4 commits March 28, 2018 14:34

Moves the shimmerInitialItems to a constant.

735c5f4

Change the language for documentation of enableShimmer.

8179102

Add comment to example.

e0560bf

Merge branch 'master' into v-vibr/DetailsList-Shimmer

1817383

ThomasMichon approved these changes Apr 2, 2018

View reviewed changes

ThomasMichon merged commit 0771cc2 into microsoft:master Apr 2, 2018

Vitalius1 deleted the v-vibr/DetailsList-Shimmer branch April 12, 2018 23:42

microsoft locked as resolved and limited conversation to collaborators Aug 31, 2019

Conversation

Vitalius1 commented Mar 23, 2018

Pull request checklist

Description of changes

Focus areas to test

Uh oh!

Vitalius1 commented Mar 23, 2018

Uh oh!

Jahnp commented Mar 23, 2018

Uh oh!

atneik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vitalius1 Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vitalius1 Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atneik Mar 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzearing commented Mar 26, 2018

Uh oh!

dzearing commented Mar 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Vitalius1 commented Mar 27, 2018

Uh oh!

Vitalius1 commented Mar 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Vitalius1 Mar 23, 2018 •

edited

Loading

Vitalius1 Mar 23, 2018 •

edited

Loading

atneik Mar 23, 2018 •

edited

Loading