Fix animation of fill prop of the svg element #33224

rayane-djouah · 2023-12-18T13:07:34Z

Details

Fixed Issues

$ #33277
$ #26654 (comment)

Tests

FAB Animation test:

Verify that the Floating action button ''+'' icon color is initially light
Verify that the Floating action button ''+'' icon color is not initially rotated
Verify that the Floating action button background color is initially green
Click on the FAB
Verify that the FAB ''+'' icon color is dark
Verify that the Floating action button ''+'' icon color is rotated
Verify that the Floating action button background color is dark
Verify that clicking the FAB toggle the animation between the two states

Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

same as tests above.

Verify that no errors appear in the JS console

PR Author Checklist

I linked the correct issue in the ### Fixed Issues section above
I wrote clear testing steps that cover the changes made in this PR
- I added steps for local testing in the Tests section
- I added steps for the expected offline behavior in the Offline steps section
- I added steps for Staging and/or Production testing in the QA steps section
- I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
- I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
I included screenshots or videos for tests on all platforms
I ran the tests on all platforms & verified they passed on:
- Android: Native
- Android: mWeb Chrome
- iOS: Native
- iOS: mWeb Safari
- MacOS: Chrome / Safari
- MacOS: Desktop
I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
I followed proper code patterns (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
- I verified that the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
  - If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
- I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
- I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I followed the guidelines as stated in the Review Guidelines
I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
I verified that if a function's arguments changed that all usages have also been updated correctly
If any new file was added I verified that:
- The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Android: Native

Android: mWeb Chrome

iOS: Native

iOS: mWeb Safari

MacOS: Chrome / Safari

MacOS: Desktop

melvin-bot · 2023-12-18T13:08:10Z

@hayata-suenaga Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

src/components/FloatingActionButton/FabPlusIcon.js

tomekzaw

Thanks for the changes! Left one suggestion.

src/components/FloatingActionButton/FabPlusIcon.js

tomekzaw

Looks good to me!

src/components/FloatingActionButton/FabPlusIcon.js

rayane-djouah · 2023-12-18T13:59:41Z

do you have any idea on why the unit test is failing ? https://github.com/Expensify/App/actions/runs/7248895066/job/19745729356

rushatgabhane · 2023-12-18T14:02:48Z

iOS error - _.keys is not a function.

Is this happening because it's running on UI thread, and a closure isn't created?
So I think you can only use shared values, reanimated funcs and native JS only.

Steps:

build
click FabIcon

rushatgabhane · 2023-12-18T14:09:36Z

@rayane-djouah let's figure out the tests after all platforms are working

rushatgabhane · 2023-12-19T00:09:42Z

we need to figure out how to fix the failing test

https://expensify.slack.com/archives/C01GTK53T8Q/p1702912750041039?thread_ts=1702883604.941629&cid=C01GTK53T8Q

rushatgabhane · 2023-12-19T11:36:55Z

@rayane-djouah we have an error on android

rayane-djouah · 2023-12-19T13:09:48Z

~~@rushatgabhane, I've found that moving the adapter definition inside the component resolved the bug. Could you please confirm this?~~

tomekzaw · 2023-12-19T13:13:40Z

@rayane-djouah This was actually my code review suggestion (#33224 (comment)) but it looks like I got it wrong, sorry about that. 😢 If it works inside the component then I think we're good to go.

Julesssss · 2023-12-19T13:39:49Z

hey, it sounds like the final issues will be resolved shortly? If yes I won't revert the PR.

src/components/FloatingActionButton/FabPlusIcon.js

rayane-djouah · 2023-12-19T13:42:10Z

my bad, sorry @tomekzaw, I thought moving the adapter was the cause because resetting to 9c1da47 fixed the error, but I was wrong.
I'm trying to figure out what is the root cause

tomekzaw · 2023-12-19T14:02:41Z

I thought moving the adapter was the cause because resetting to 9c1da47 fixed the error

Do you mean it fixed the original error (the one with Double) or the failing tests? Surprisingly, the app works fine for me both with adapter inside and outside of the component. No idea how to fix failing tests, though.

rayane-djouah · 2023-12-19T14:11:46Z

actually the bug on android cause was the changes made to the adapter in this commit 225988e
I had to ignore this lint error:

Error: 29:17 error Assignment to property of function parameter 'props' no-param-reassign here

…SVG-element

rayane-djouah · 2023-12-19T14:21:28Z

@Julesssss the crash on android bug is fixed now, but we have a failing ui test remaining.
https://github.com/Expensify/App/actions/runs/7263101931/job/19787666391?pr=33224

situchan · 2023-12-19T14:28:46Z

As unit test is still failing and not found solution yet, I think we should still go revert approach as QA team is not able to test staging android at all. Expecting fix much earlier than deploy time as QA time required.

rayane-djouah · 2023-12-19T16:30:50Z

@tomekzaw calling createAnimatedPropAdapter is causing the test to fail:

Recording.2023-12-19.172301.mp4

any ideas on this?

rushatgabhane · 2023-12-19T17:39:21Z

not too familiar but could we mock createAnimatedPropAdapter ? @rayane-djouah

rayane-djouah · 2023-12-19T18:17:16Z

Thank you @rushatgabhane the test is passing now

rushatgabhane · 2023-12-19T18:31:58Z

@rayane-djouah
Bug: On web, the Fab icon + has black color.

Staging has white color

rayane-djouah · 2023-12-19T21:53:28Z

src/components/FloatingActionButton/FabPlusIcon.js

+        // eslint-disable-next-line rulesdir/prefer-underscore-method
+        if (Object.keys(props).includes('fill')) {
+            // eslint-disable-next-line no-param-reassign
+            props.fill = {type: 0, payload: processColor(props.fill)};


@rayane-djouah Bug: On web, the Fab icon + has black color.

Staging has white color

I think that the bug cause is that we are not adapting properly the fill property from react-native-svg here, The fill color defaults to black.
using the example from the docs is not working.
we should understand the expected color structure, and how we should set the props.fill in this case. for this, we need more information from SWM on the ColorStruct type declaration here, and what color format is expected by processColor.
cc @tomekzaw

situchan · 2023-12-19T22:17:58Z

Is this ready to merge? If not, no need to hesitate revert as original issue is just function migration.
QA team is blocked android testing for full day: #33264 (comment)
cc: @jasperhuangg

rayane-djouah · 2023-12-19T22:43:52Z

@situchan I created a revert PR here if we want to revert the PR

rayane-djouah · 2023-12-19T22:51:31Z

we can either revert the PR or merge this PR and fix this bug in a follow up PR

jasperhuangg · 2023-12-20T02:27:19Z

I went ahead and merged the revert @rayane-djouah

mountiny · 2023-12-20T08:40:05Z

@rayane-djouah Should we close this PR and handle the fix in the new attempt?

tomekzaw · 2023-12-20T13:55:27Z

Regarding the black color on web, actually adapter is needed only on the native platforms and should not be used on web. So here's the patch:

     const animatedProps = useAnimatedProps(
         () => {
             const fill = interpolateColor(sharedValue.value, [0, 1], [textLight, textDark]);
 
             return {
                 fill,
             };
         },
-        [],
-        adapter
+        undefined,
+        Platform.OS === 'web' ? undefined : adapter,
     );

After applying this change, the initial color of the plus icon is white. However, the animation does not run on web. The root cause is that the component is unmounted and mounted again on each change of isActive (on web), however on native it stays the same. You can confirm it by adding the following code snippet somewhere in FabPlusIcon component:

useEffect(() => {
  console.log('mounted');
  return () => console.log('unmounted');
}, []);

rayane-djouah added 2 commits December 18, 2023 13:32

Fix animation of fill prop of the SVG element

2f66190

fix error: importing createAnimatedPropAdapter

92f3b54

rayane-djouah requested a review from a team as a code owner December 18, 2023 13:07

melvin-bot bot requested review from hayata-suenaga and removed request for a team December 18, 2023 13:08

rayane-djouah mentioned this pull request Dec 18, 2023

Migrate Icon/index.js and FloatingActionButton to function component #26654

Merged

58 tasks

VickyStash mentioned this pull request Dec 18, 2023

[TS migration] Migrate '[Remaining Group 2]' lib to TypeScript #32164

Merged

50 tasks

rayane-djouah added 2 commits December 18, 2023 14:20

fix lint error

9c1da47

fix error: The fill color of the plus icon does not get animated

225988e

rayane-djouah commented Dec 18, 2023

View reviewed changes

src/components/FloatingActionButton/FabPlusIcon.js Outdated Show resolved Hide resolved

fix propTypes declaration for the adapter

9042025

tomekzaw reviewed Dec 18, 2023

View reviewed changes

src/components/FloatingActionButton/FabPlusIcon.js Outdated Show resolved Hide resolved

move adapter outside of the component

adbb866

tomekzaw reviewed Dec 18, 2023

View reviewed changes

rushatgabhane reviewed Dec 18, 2023

View reviewed changes

src/components/FloatingActionButton/FabPlusIcon.js Outdated Show resolved Hide resolved

remove unnecessary comment

62833d1

rayane-djouah added 2 commits December 18, 2023 15:55

use native Object.keys instead of _.keys

bf164fd

ignore prefer-underscore-method lint rule

ccec8cf

Julesssss assigned rayane-djouah Dec 19, 2023

Julesssss mentioned this pull request Dec 19, 2023

[$500] Android - App crashes , unable to open the app #33277

Closed

6 tasks

move adapter inside the component

2cf48f5

fix error on android

28b8b72

tomekzaw suggested changes Dec 19, 2023

View reviewed changes

src/components/FloatingActionButton/FabPlusIcon.js Outdated Show resolved Hide resolved

rayane-djouah added 2 commits December 19, 2023 15:04

remove duplicate definition

abf9500

correct eslint-disable-next-line comment

e1e2015

rayane-djouah added 3 commits December 19, 2023 15:14

correct eslint-disable-next-line comment

6487dcf

Merge branch 'Expensify:main' into Fix-animation-of-fill-prop-of-the-…

6007567

…SVG-element

correct eslint-disable-next-line comment

c12ad0f

mock createAnimatedPropAdapter from react-native-reanimated

b57637d

rayane-djouah requested a review from rushatgabhane December 19, 2023 18:21

rayane-djouah commented Dec 19, 2023

View reviewed changes

rayane-djouah closed this Dec 20, 2023

rayane-djouah deleted the Fix-animation-of-fill-prop-of-the-SVG-element branch December 20, 2023 10:11

rayane-djouah mentioned this pull request Dec 20, 2023

Migrate Icon and FloatingActionButton to function component #33336

Merged

50 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix animation of fill prop of the svg element #33224

Fix animation of fill prop of the svg element #33224

rayane-djouah commented Dec 18, 2023 •

edited

Loading

melvin-bot bot commented Dec 18, 2023

tomekzaw left a comment

tomekzaw left a comment

rayane-djouah commented Dec 18, 2023

rushatgabhane commented Dec 18, 2023 •

edited

Loading

rushatgabhane commented Dec 18, 2023 •

edited

Loading

rushatgabhane commented Dec 19, 2023

rushatgabhane commented Dec 19, 2023 •

edited

Loading

rayane-djouah commented Dec 19, 2023 •

edited

Loading

tomekzaw commented Dec 19, 2023 •

edited

Loading

Julesssss commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

tomekzaw commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023 •

edited

Loading

situchan commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

rushatgabhane commented Dec 19, 2023 •

edited

Loading

rayane-djouah commented Dec 19, 2023

rushatgabhane commented Dec 19, 2023 •

edited

Loading

rayane-djouah Dec 19, 2023

situchan commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

jasperhuangg commented Dec 20, 2023

mountiny commented Dec 20, 2023

tomekzaw commented Dec 20, 2023 •

edited

Loading

Fix animation of fill prop of the svg element #33224

Fix animation of fill prop of the svg element #33224

Conversation

rayane-djouah commented Dec 18, 2023 • edited Loading

Details

Fixed Issues

Tests

Offline tests

QA Steps

PR Author Checklist

Screenshots/Videos

melvin-bot bot commented Dec 18, 2023

tomekzaw left a comment

Choose a reason for hiding this comment

tomekzaw left a comment

Choose a reason for hiding this comment

rayane-djouah commented Dec 18, 2023

rushatgabhane commented Dec 18, 2023 • edited Loading

rushatgabhane commented Dec 18, 2023 • edited Loading

rushatgabhane commented Dec 19, 2023

rushatgabhane commented Dec 19, 2023 • edited Loading

rayane-djouah commented Dec 19, 2023 • edited Loading

tomekzaw commented Dec 19, 2023 • edited Loading

Julesssss commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

tomekzaw commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023 • edited Loading

situchan commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

rushatgabhane commented Dec 19, 2023 • edited Loading

rayane-djouah commented Dec 19, 2023

rushatgabhane commented Dec 19, 2023 • edited Loading

rayane-djouah Dec 19, 2023

Choose a reason for hiding this comment

situchan commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

rayane-djouah commented Dec 19, 2023

jasperhuangg commented Dec 20, 2023

mountiny commented Dec 20, 2023

tomekzaw commented Dec 20, 2023 • edited Loading

rayane-djouah commented Dec 18, 2023 •

edited

Loading

rushatgabhane commented Dec 18, 2023 •

edited

Loading

rushatgabhane commented Dec 18, 2023 •

edited

Loading

rushatgabhane commented Dec 19, 2023 •

edited

Loading

rayane-djouah commented Dec 19, 2023 •

edited

Loading

tomekzaw commented Dec 19, 2023 •

edited

Loading

rayane-djouah commented Dec 19, 2023 •

edited

Loading

rushatgabhane commented Dec 19, 2023 •

edited

Loading

rushatgabhane commented Dec 19, 2023 •

edited

Loading

tomekzaw commented Dec 20, 2023 •

edited

Loading