feat: add AnimatedFAB improvements by Drakeoon · Pull Request #2907 · callstack/react-native-paper

Drakeoon · 2021-09-20T13:33:21Z

Summary

This PR introduces fixes and improvements to original AnimatedFAB pull request

Add support for animateFor: "left" property
Add snapshot tests for AnimatedFAB component
~~Add integration tests for AnimatedFAB component (new properties)~~

Test plan

iOS

AnimatedFAB.iOS.mp4

Android

AnimatedFAB.Android.mp4

callstack-bot · 2021-09-20T13:38:34Z

Hey @Drakeoon, thank you for your pull request 🤗. The documentation from this branch can be viewed here.

Drakeoon · 2021-09-20T14:14:32Z

@lukewalczak I noticed two things:

The AnimatedFAB behavior is different depending on the platform. On iOS it expands when we scroll up and collapses when we scroll down. On Android it does the opposite - expands when we scroll down and collapses when we scroll up. Is this an intended behavior?
Pressing on the button does not show the ripple effect as on your video. It could be a small bug introduced at some point during development, I'll try to investigate futher

lukewalczak · 2021-09-20T14:52:54Z

The AnimatedFAB behavior is different depending on the platform. On iOS it expands when we scroll up and collapses when we scroll down. On Android it does the opposite - expands when we scroll down and collapses when we scroll up. Is this an intended behavior?

Pressing on the button does not show the ripple effect as on your video. It could be a small bug introduced at some point during development, I'll try to investigate futher

On both platforms behavior should be the same. According to the documentation:

The FAB should not return to an extended FAB until the user scrolls back to the top of the page.

like on the video from MD

There should be a ripple effect on the whole button.

…ct-native-paper into feat/animated-extended-fab-improvements

p-syche

Great job improving this component!
I left you a few comments to consider, and a few nitpicks ;)
One important note is that you have a FlatList without keys which definitely needs to be fixed. I also don't love the hardcoded SIZE.
Other than that this is solid work, nice usage of types 👍

p-syche · 2021-10-14T15:59:04Z

+  /**
+   * Label for extended `FAB`.
+   */
+  label: string;


It would be preferable to add some string length constraints to your label.

That's a good idea, however we didn't yet choose what the allowed label size is. I think for now we'll allow any size and in future iterations we can improve on that. WDYT @lukewalczak ?

Actually, I think we should leave that responsibility to the user.

p-syche · 2021-10-14T16:02:10Z

+  testID?: string;
+};
+
+const SIZE = 56;


Hardcoded size may become a real headache when Apple comes up with a new super small or super-duper large display 😂 Consider using something more future-proof, for example check screen size and calculate a percentage of it.

I agree with you. The size comes from the Material Design Guidelines, but I'm also aware that we use many hacks to just make it "look good". While I think we shouldn't make the Icon responsive (nor derive it from screen), I think we still could do a better job and avoid hacks. I'll leave it as it is, but for sure this could be improved in the future

p-syche · 2021-10-14T16:05:10Z

+        ...defaultPositionStyles,
+      },
+      absoluteFill: {},
+    };


I'm not sure I see the point of creating CombinedStyles, it adds a bit of complexity. Can you tell me why you decided to create this const this way?

My main goal was to move dynamic styles logic to a function, that I can later move to a separate file to clean up the JSX and component logic. Thanks for pointing this out, as I forgot to do it before requesting code review 😄 Please take a look at my recent changes

Looks much cleaner now 👍

p-syche · 2021-10-14T16:07:48Z

+          }),
+        },
+      ];
+    }


This whole block may be more readable as a switch statement instead of multiple ifs.

I've tried implementing it with switch statement, but I think it's kind of the same. Ideally I would use something like Immer here to mutate the objects, but that's what we're left with for now

…ct-native-paper into feat/animated-extended-fab-improvements

Drakeoon · 2021-10-21T11:12:26Z

@p-syche Thank you so much for your feedback! I really appreciate it 👍🏻

I answered your questions and implemented requested changes. There are also some small refactor commits to improve code readability. I think the PR is ready for your final review @p-syche @lukewalczak

p-syche · 2021-10-21T23:02:32Z

@Drakeoon thank you for following up on my notes.
@lukewalczak I think this is good to go, but I don't want to pull the trigger without your blessing. Also a maintainer's approval is necessary so I can't pull the trigger 😅

lukewalczak · 2021-10-25T10:47:35Z

@Drakeoon could you please also:

remove DEV check from example list
remove filter from docs

to unblock that component in prod?

lukewalczak

Awesome work 🎉 !

ice-chillios reviewed Sep 21, 2021

View reviewed changes

Comment thread src/components/FAB/AnimatedFAB.tsx Outdated

Drakeoon force-pushed the feat/animated-extended-fab-improvements branch from 0d96eb4 to 184ce29 Compare September 23, 2021 14:23

lukewalczak and others added 25 commits September 24, 2021 12:01

feat: introduce first iteration of AnimatedFAB component

6365c2c

refactor: remove useless code

ca0617d

refactor: don't move fab when switching animation direction prop

b05e782

refactor: interpolate animation on fab text

c397460

refactor: code and example clean up

f0ae001

refactor: use one animated value

554e79a

feat: configure shadow for Android

0049b6d

refactor: introduce isIOS

396e775

refactor: fixes after merge

fb69d42

refactor: adjust android example

06414ee

refactor: small changes

9f77407

fix: add support for animateFor: left property

4e99ffd

test: add snapshot tests for AnimatedFAB component

7adecfc

test: add snapshot tests for new properties

20b5913

feat: tweak example to match MD docs

c84999c

feat: improve AnimatedFAB example for iOS

7c7cc82

feat: improve AnimateFAB visuals

c0de872

feat: improve example to showcase animateFrom prop

684c918

feat: add support for RTL and fix animateFrom prop

3221e19

fix: align iOS behavior for AnimatedFAB example

77db8d4

feat: improve AnimatedFAB example

20e1481

feat: implement consistent styles with a dirty approach

67e1ef5

chore: refactor innerWrapper styles

d241783

chore: refactor icon styles

f2030fc

chore: refactor reversed properties for direction

3ebb9e6

lukewalczak added 4 commits October 11, 2021 11:36

refactor: introduce isIOS

0069512

refactor: fixes after merge

73a71fc

refactor: adjust android example

f01ca7d

refactor: small changes

a573c04

lukewalczak force-pushed the feat/animated-extended-fab branch from 80fa1f5 to a573c04 Compare October 11, 2021 09:36

Drakeoon and others added 2 commits October 11, 2021 12:35

Merge branch 'feat/animated-extended-fab' of github.com:callstack/rea…

5af9ef8

…ct-native-paper into feat/animated-extended-fab-improvements

feat: hide AnimatedFab for production

fcaefef

lukewalczak force-pushed the feat/animated-extended-fab branch from a637f14 to fcaefef Compare October 13, 2021 14:46

p-syche suggested changes Oct 14, 2021

View reviewed changes

Merge branch 'feat/animated-extended-fab' of github.com:callstack/rea…

39f8ede

…ct-native-paper into feat/animated-extended-fab-improvements

lukewalczak deleted the branch callstack:main October 19, 2021 07:46

lukewalczak closed this Oct 19, 2021

lukewalczak mentioned this pull request Oct 19, 2021

feat: introduce first iteration of AnimatedFAB component #2580

Merged

lukewalczak reopened this Oct 19, 2021

lukewalczak changed the base branch from feat/animated-extended-fab to main October 19, 2021 07:53

Drakeoon added 5 commits October 19, 2021 09:56

refactor: apply small feedback notes

ceb03cb

Merge branch 'main' into feat/animated-extended-fab-improvements

f240cff

refactor: apply small feedback notes

c897aa0

refactor: move dynamic styles to a separate utils file

7aa93e7

refactor: move AnimatedFAB files to a separate folder

6a56b0d

Drakeoon requested a review from p-syche October 21, 2021 11:12

refactor: export missing types from the package

c968eda

chore: enable AnimateFAB in production

d05dc7b

Drakeoon changed the title ~~AnimatedFAB improvements~~ feat: add AnimatedFAB improvements Oct 29, 2021

lukewalczak reviewed Nov 3, 2021

View reviewed changes

lukewalczak approved these changes Nov 3, 2021

View reviewed changes

lukewalczak merged commit 50ccce0 into callstack:main Nov 3, 2021

Uh oh!

Conversation

Drakeoon commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

iOS

Android

Uh oh!

callstack-bot commented Sep 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Drakeoon commented Sep 20, 2021

Uh oh!

lukewalczak commented Sep 20, 2021

Uh oh!

Uh oh!

p-syche left a comment

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!

Drakeoon Oct 21, 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!

Drakeoon commented Oct 21, 2021

Uh oh!

p-syche commented Oct 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lukewalczak commented Oct 25, 2021

Uh oh!

lukewalczak left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Drakeoon commented Sep 20, 2021 •

edited

Loading

callstack-bot commented Sep 20, 2021 •

edited

Loading

Drakeoon Oct 21, 2021 •

edited

Loading

p-syche commented Oct 21, 2021 •

edited

Loading