Experimental Shimmer #669

chiuam · 2021-04-29T03:28:08Z

Platforms Impacted

Description of changes

This PR removes the iOS native shimmer module and implements a new experimental component. The component uses the Animated library from react-native for animation effects, and react-native-svg for gradient and Svgs elements. It's not supported on win32 due to incompatibility with NetUI (app freezes) and absence of a platform native driver - performance takes a big hit if we don't leverage it.

Usage

<Shimmer elements={circle} width={500} />
The prop 'elements' takes an array of ShimmerElement objects to build a shimmer view with one or more ShimmerElementType (rect / line, circle). Note: image is not supported as a sub-element.
<Shimmer height={10} />
If no element is passed in, the component will return a rect shimmer by default.

Verification

shimmer-ios-light.mov

shimmer-macos-light.mov

shimmer-android-dark.mov

Pull request checklist

This PR has considered (when applicable):

* initial android theme * mapped a lot of colors, updated readme, added to fluent-tester-app * Change files * removed unneccessary dependencies * added android typography * updated versioning, removed apple specific changes * making android-theme default for the platform * update fluent-tester dependency version... * resolving comments... * resolving more comments... Co-authored-by: Tushar Masane <[email protected]>

* Change files * Remove change file * Add repository link * Change files

* Add a nil check to AvatarData image * Change files

…osoft#653) * Change files * Remove change file * Update radio button focus border style * Change files

microsoft#654) * Add the rainbow gradient test case * Change files

* Add apple theme static libraries to NuGet, add NuGet Publish PR job * Fix path in nuspec * Comment out NugetPublish PR Job

* fix typos in android-theme * Change files

* added svg, transformer to android * base64 svgs do not load, font files do not link * Change files * bump package versions manually * Change files * upgrade react-native-svg * Change files * revert 'bump package versions manually' * Change files

* Fix typo in nuspec * Comment out NuGet Publish again

* Add macOS support * Add link to issue regarding direct SVG imports * Replace react-native-svg with @microsoft/react-native-svg-desktop * Use react-native-svg now that it supports macOS * Upgrade react-native-svg to 12.1.1-0 everywhere * Update macos podfile * Change files

`rnx-start` is being removed because it currently does not provide any value over vanilla `react-native start`. See microsoft/rnx-kit#139

* Fix typos, uncomment pipeline * comment out pipeline again

* updated themepicker for android * Change files * add correct picker... * ignore rn-picker depcheck... * separated android themepicker... * added dropdown styles... * add types for dropdown..

* Initial Commit * Restructuring * Rewrite Slots summary * Update Guide based on feedback * More fixes * Update from PR feedback

…rosoft#668) * Add button icon tests specific to win32 * Change files * Change files * Add dependency to rnsvg * Add RNSVG dependency to ios and mac * Remove rnsvg dep from ios and macos due to failed depcheck * Add rnsvg to ios and macos againt but also add to ignores for depcheck

* Add direct dependency on RNSVG in FURN Icon * Change files * Update yarn.lock with react-native-svg 12.1.1

packages/experimental/Shimmer/src/Shimmer.tsx

Saadnajmi · 2021-04-29T05:23:52Z

packages/experimental/Shimmer/tsconfig.json

  },
  "include": ["src"]
-}
+}


nit: newline? If only so that symbol goes away 😅

packages/experimental/Shimmer/src/Shimmer.tsx

packages/experimental/Shimmer/src/Shimmer.types.ts

packages/experimental/Shimmer/package.json

packages/experimental/Shimmer/src/Shimmer.tsx

package-lock.json

apps/fluent-tester/package.json

chiuam · 2021-05-03T17:12:21Z

apps/fluent-tester/src/FluentTester/TestComponents/Shimmer/ShimmerTest.tsx

-    shimmerDelay: 0.1,
-    viewTintColor: 'rgb(100, 100, 100)',
-    cornerRadius: 10,
+    gradientTintColor: "pink",


Token customization isn't working for some reason...

packages/experimental/Shimmer/src/Shimmer.styling.ts

packages/experimental/Shimmer/src/Shimmer.types.ts

Saadnajmi · 2021-05-03T21:38:09Z

packages/experimental/Shimmer/src/Shimmer.types.ts

+   * Specifies the gradient angle, value should be less than 1
+   * @defaultValue '0'
+   */
+  angle?: number;


if less than 1, what units are these in? radians? does 0->1 map from 0 degrees to 90 degrees?

I'm not sure about the units tbh. For the mapping, y1 should be anywhere from -1 to 1, -1 being 90 degrees. I think this is something we can iterate on though.

packages/experimental/Shimmer/src/Shimmer.types.ts

apps/fluent-tester/package.json

apps/fluent-tester/src/FluentTester/TestComponents/Shimmer/ShimmerTest.tsx

packages/experimental/Shimmer/src/Shimmer.types.ts

packages/experimental/Shimmer/src/Shimmer.tsx

PPatBoyd · 2021-05-03T21:55:55Z

packages/experimental/Shimmer/src/Shimmer.tsx

+          toValue: memoData.toValue,
+          duration: memoData.duration,
+          delay: memoData.delay,
+          useNativeDriver: false,


useNativeDriver = true?

There's a bug related to looping with the native driver microsoft/react-native-macos#763
There's an open PR in Facebook upstream that fixes it, so I think Amanda was working on bringing it to react-native-macos before turning it on.

PPatBoyd · 2021-05-03T21:58:29Z

packages/experimental/Shimmer/src/Shimmer.tsx

+      }
+      return (
+        <Slots.root {...mergedProps}>
+          <Defs>


@ startValue, I recall chatting about it a bit but this causes the animation to run on the js driver the entire time and consistently churn the bridge and the SVG graphic, no? Did we need to bail from an impl where there's a separate svg clipping mask from the gradient?

Very good point. Since startValue uses the state hook which causes re-render on every state change, this does cause performance issues. I learned that useRef does the same thing as useState - preserves the data after a re-render, except it doesn't cause re-render. Since we are not updating the component on state change, useRef is probably the more suitable approach here.

Talked offline. I'm still not sure if rendering the animation redraws the SVG, opened #675 to track this.

packages/experimental/Shimmer/src/Shimmer.tsx

Saadnajmi · 2021-05-06T19:08:35Z

packages/experimental/Shimmer/src/Shimmer.tsx

+          {uri && <Slots.image href={props.uri} />}
+          <ClipPath id="shimmerView">
+            {rows}
+            {rect && <Slots.rect width={props.rect.width} x={props.rect.x} y={props.rect.y} />}


Was there a strong reason to design the API so that these elements are props passed in instead of children things ? I know you spent a lot of time looking into the design of that, so I was curious where that ended up.
My impression right now is you very strongly wanted to have the shimmer only have these 3 elements in no other configuration. Is that correct?

Yeah, I thought it'd be cleaner for the client to construct a group view with props, also I believe that's how Fluent web does it too.

x y , cx cy etc are going to give us the position details, I don't think that should matter here -- so long as that's ending up as a single svg layer, we shouldn't need more than one clipping mask [not 100% clear here but also technically flexible not doublechecking atm :D]

rows has the details for arbitrary shape counts, though do we actually need the {rect && ..} and {circle && ..} Amanda?

Ah, I see that FluentUI React Shimmer does it with the props, and FluentUI React Northstar Skeleton does it with children. That's probably why I got slightly confused. Thanks!

Thanks for sharing the northstar version, I didn't know about it! One thing I like about using props is that the sub-elements are reusable on the client side, kinda like the example I have in the test page, (not sure if that's possible with children). I have a feeling that the northstar version should be the gold standard we should follow though...

Reusable in that you can use the same element in multiple shimmers? Or that You can render an individual element? I think there's ways to do both. I also agree that Northstar is probably the thing to base off of, but I get that's a fairly big API change too and probably out of scope if we already like our props design

ugh I keep resolving the conversation trying to reply, there's so much about github UI that I wish was... different :(

TBH I don't know for sure that I think the northstar version is how we want to do it -- I know we talked about it [sorry random person I tagged!] @chiuam but I don't recall what all we said :D We definitely want to be as consistent as possible w.r.t. general API and functionality, but impl details are up for optimizing between platforms.

As northstar's implemented straight ported, we'd have an element per clipping element in layout which feels wasteful to me, minding impl maintenance (cutting shapes on a layer rect vs converting to layout props) and performance (memory, layout runtime cost). The only advantage that comes to mind is being able to animate shapes individually, which isn't spec'd or as far as I know in anyone's ideal api.

One consideration is that 'children' doesn't necessarily mean native elements for us here, the format is really just an interface, but it may introduce important limitations in implementation options js +- native sides.

I'll start a quick email with them to get their thoughts, there may be web specifics (e.g. underlying platform implications) or advantages to the API that aren't coming to mind at the moment -- or ya'll let me know if you've already been thinking about some :)

The former. I'm not sure what the advantages of using children are aside from readability, I'm just leaning towards props for now until we have stronger reasons to do otherwise.

I think the main benefit is clients have stronger control over the children, where they can style each child individually / layout circles and lines and rects how they see fit.
It sounds like we weren't going for that with this Shimmer, though I still think it's a useful followup in the future, if it comes up as a client need.

@Saadnajmi my assertion is Shimmer is one of our most opinionated controls, and except for maybe something like "provide your own clipping mask" I'm [personally] not interested in offering that level of customization. It's way more open to bugs and maintenance issues than we want to be e.g. I don't want debugging to include layout, or take the perf cost of layout and unnecessary elements. Let someone else extend Shimmer into another control or write their own control [not extending] at that point because they're doing something quite specialized.

Part of my overall view here is JSX is just syntactic sugar for this control, and we only want 2 internal elements: clipping mask and gradient, the gradient animating underneath via translation. (image or similar as content underneath I'm more open to but also don't see the need for compared to consumer doing it themselves). Having individual elements implies [to me] black magic native coordination (black magic native bug potential) or undesired visuals (disconnected gradients), undesirable/unnecessary UI elements and bugs (what happens when they make a control focusable?), and potentially complications for animations. We only want to translate for gradient motion, we don't want to be re-drawing the gradient every tick.

Saadnajmi

LGTM!

packages/experimental/Shimmer/src/Shimmer.types.ts

PPatBoyd · 2021-05-03T22:08:16Z

packages/experimental/Shimmer/src/Shimmer.types.ts

+   * Specifies the gradient opacity, value should be less than 1
+   * @defaultValue '.7'
+   */
+  gradientOpacity?: number;


Did we have thoughts about specifying opacity vs letting that be handled by ARGB color stops? I'm inclined against an opacity prop but don't recall prior conversations

packages/experimental/Shimmer/src/Shimmer.types.ts

PPatBoyd · 2021-05-06T19:33:07Z

packages/experimental/Shimmer/src/Shimmer.tsx

+          {uri && <Slots.image href={props.uri} />}
+          <ClipPath id="shimmerView">
+            {rows}
+            {rect && <Slots.rect width={props.rect.width} x={props.rect.x} y={props.rect.y} />}


x y , cx cy etc are going to give us the position details, I don't think that should matter here -- so long as that's ending up as a single svg layer, we shouldn't need more than one clipping mask [not 100% clear here but also technically flexible not doublechecking atm :D]

rows has the details for arbitrary shape counts, though do we actually need the {rect && ..} and {circle && ..} Amanda?

PPatBoyd · 2021-05-06T20:39:23Z

packages/experimental/Shimmer/src/Shimmer.tsx

+          {uri && <Slots.image href={props.uri} />}
+          <ClipPath id="shimmerView">
+            {rows}
+            {rect && <Slots.rect width={props.rect.width} x={props.rect.x} y={props.rect.y} />}


ugh I keep resolving the conversation trying to reply, there's so much about github UI that I wish was... different :(

TBH I don't know for sure that I think the northstar version is how we want to do it -- I know we talked about it [sorry random person I tagged!] @chiuam but I don't recall what all we said :D We definitely want to be as consistent as possible w.r.t. general API and functionality, but impl details are up for optimizing between platforms.

As northstar's implemented straight ported, we'd have an element per clipping element in layout which feels wasteful to me, minding impl maintenance (cutting shapes on a layer rect vs converting to layout props) and performance (memory, layout runtime cost). The only advantage that comes to mind is being able to animate shapes individually, which isn't spec'd or as far as I know in anyone's ideal api.

One consideration is that 'children' doesn't necessarily mean native elements for us here, the format is really just an interface, but it may introduce important limitations in implementation options js +- native sides.

I'll start a quick email with them to get their thoughts, there may be web specifics (e.g. underlying platform implications) or advantages to the API that aren't coming to mind at the moment -- or ya'll let me know if you've already been thinking about some :)

PPatBoyd · 2021-05-07T19:59:53Z

packages/experimental/Shimmer/src/Shimmer.tsx

+  Animated.loop(
+    Animated.sequence([
+    Animated.timing(startValue, {
+      toValue: memoData.toValue,


nit better names that toValue, startValue?

w/ comments for how it's used

PPatBoyd · 2021-05-07T20:16:24Z

packages/experimental/Shimmer/src/Shimmer.types.ts

+}
+
+export interface ShimmerTokens extends ShimmerPropTokens {
+  viewBox?: string;


what does this mean actually? didn't notice this before

PPatBoyd · 2021-05-07T20:54:03Z

packages/experimental/Shimmer/src/Shimmer.tsx

+          {uri && <Slots.image href={props.uri} />}
+          <ClipPath id="shimmerView">
+            {rows}
+            {rect && <Slots.rect width={props.rect.width} x={props.rect.x} y={props.rect.y} />}


@Saadnajmi my assertion is Shimmer is one of our most opinionated controls, and except for maybe something like "provide your own clipping mask" I'm [personally] not interested in offering that level of customization. It's way more open to bugs and maintenance issues than we want to be e.g. I don't want debugging to include layout, or take the perf cost of layout and unnecessary elements. Let someone else extend Shimmer into another control or write their own control [not extending] at that point because they're doing something quite specialized.

Part of my overall view here is JSX is just syntactic sugar for this control, and we only want 2 internal elements: clipping mask and gradient, the gradient animating underneath via translation. (image or similar as content underneath I'm more open to but also don't see the need for compared to consumer doing it themselves). Having individual elements implies [to me] black magic native coordination (black magic native bug potential) or undesired visuals (disconnected gradients), undesirable/unnecessary UI elements and bugs (what happens when they make a control focusable?), and potentially complications for animations. We only want to translate for gradient motion, we don't want to be re-drawing the gradient every tick.

HeyImChris · 2021-05-11T00:37:34Z

packages/experimental/Shimmer/package.json

-  "author": "Microsoft <[email protected]>",
-  "homepage": "https://github.com/microsoft/fluentui-react-native",
+  "name": "@fluentui-react-native/experimental-shimmer",
+  "version": "0.1.0",


Could there be any issues with publishing if we reset the version back down to one that we already published?

We talked about it a bit in this thread (#669 (comment)) I think we landed with crossing that bridge when we get there. FWIW, "experimental-shimmer" is a new package from the "shimmer" that I wrote earlier

True, with the rename it's probably fine then

HeyImChris · 2021-05-11T02:42:21Z

packages/experimental/Shimmer/package.json

-  "author": "Microsoft <[email protected]>",
-  "homepage": "https://github.com/microsoft/fluentui-react-native",
+  "name": "@fluentui-react-native/experimental-shimmer",
+  "version": "0.1.0",


True, with the rename it's probably fine then

Saadnajmi · 2021-05-11T03:29:55Z

I just remembered. This will cause the NuGet pipeline to fail because it will try to pack the static library for the original Shimmer but not find it because we deleted it :(

NuGet pipeline was broken anyway so I guess we should go fix it : ' )

chiuam and others added 30 commits March 30, 2021 14:48

prototype

365bc4c

style gradient

dc88091

resolve conflicts

ac0595f

componentize shimmer

1874f74

prototype

f604b5a

style gradient

028d490

📦 applying package updates ***NO_CI***

28bbaed

Add repository link for @fluentui/react-native (microsoft#649)

9b32a5c

* Change files * Remove change file * Add repository link * Change files

Add a nil check to Avatar image property (microsoft#650)

21e152c

* Add a nil check to AvatarData image * Change files

Update Radio Button focus border style to be WCAG 2.1 compliant (micr…

ff066db

…osoft#653) * Change files * Remove change file * Update radio button focus border style * Change files

Add the rainbow gradient border image test case to experimental avatar (

2af0f40

microsoft#654) * Add the rainbow gradient test case * Change files

Add apple theme static libraries to NuGet package (microsoft#655)

785b104

* Add apple theme static libraries to NuGet, add NuGet Publish PR job * Fix path in nuspec * Comment out NugetPublish PR Job

Fix npm publish pipeline (hopefully) (microsoft#656)

5854ccc

* fix typos in android-theme * Change files

📦 applying package updates ***NO_CI***

5026ba4

Android Svg Support (microsoft#652)

b069f2e

* added svg, transformer to android * base64 svgs do not load, font files do not link * Change files * bump package versions manually * Change files * upgrade react-native-svg * Change files * revert 'bump package versions manually' * Change files

📦 applying package updates ***NO_CI***

5292a5f

Fix typo in nuspec (microsoft#659)

c7353d1

* Fix typo in nuspec * Comment out NuGet Publish again

📦 applying package updates ***NO_CI***

d86d656

chore: rnx-start is deprecated (microsoft#660)

367e77d

`rnx-start` is being removed because it currently does not provide any value over vanilla `react-native start`. See microsoft/rnx-kit#139

Fix more typos in the nuspec (microsoft#661)

1e2fb24

* Fix typos, uncomment pipeline * comment out pipeline again

Revamp Android ThemePicker (microsoft#664)

6498ab5

* updated themepicker for android * Change files * add correct picker... * ignore rn-picker depcheck... * separated android themepicker... * added dropdown styles... * add types for dropdown..

Add a Contributing markdown file (microsoft#555)

07d0229

* Initial Commit * Restructuring * Rewrite Slots summary * Update Guide based on feedback * More fixes * Update from PR feedback

Add explicit react-native-svg dependency to FURN Icon (microsoft#666)

f1bffad

* Add direct dependency on RNSVG in FURN Icon * Change files * Update yarn.lock with react-native-svg 12.1.1

componentize shimmer

b9f19af

pull in latest changes

989ec8a

move shimmer to experimental

54a8eb5

create slots for shapes

28666e0

chiuam changed the title ~~Experimental~~ Experimental Shimmer Apr 29, 2021

chiuam marked this pull request as draft April 29, 2021 03:30

Saadnajmi reviewed Apr 29, 2021

View reviewed changes

package-lock.json Outdated Show resolved Hide resolved

apps/fluent-tester/package.json Show resolved Hide resolved

chiuam added 4 commits April 29, 2021 18:51

revamp shimmer slots

67c8abe

implement shimmerElement to group sub-elements

fa399d5

Change files

72b513d

add shimmer to android and accessibility support

c4d11cf

chiuam marked this pull request as ready for review May 3, 2021 15:52

chiuam commented May 3, 2021

View reviewed changes

Saadnajmi reviewed May 3, 2021

View reviewed changes

chiuam mentioned this pull request May 3, 2021

Fix animation bug microsoft/react-native-macos#763

Merged

4 tasks

PPatBoyd requested changes May 3, 2021

View reviewed changes

Saadnajmi reviewed May 6, 2021

View reviewed changes

chiuam added 3 commits May 6, 2021 15:10

use useRef hook and union types

1aacd69

remove rect & circle slots

51765dc

documentation

c6343a0

Saadnajmi approved these changes May 10, 2021

View reviewed changes

PPatBoyd approved these changes May 10, 2021

View reviewed changes

HeyImChris reviewed May 11, 2021

View reviewed changes

HeyImChris approved these changes May 11, 2021

View reviewed changes

HeyImChris merged commit e2d6647 into microsoft:master May 11, 2021

Experimental Shimmer #669

Experimental Shimmer #669

Uh oh!

Conversation

chiuam commented Apr 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Platforms Impacted

Description of changes

Usage

Verification

Pull request checklist

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chiuam May 6, 2021 • 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!

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!

Saadnajmi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

chiuam commented Apr 29, 2021 •

edited

Loading

chiuam May 6, 2021 •

edited

Loading