Skip to content
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.

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

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

Conversation

rayane-djouah
Copy link
Contributor

@rayane-djouah rayane-djouah commented Sep 4, 2023

Details

Migrate Icon/index.js to function component.

Migrate FloatingActionButton.js to function component.

Moved FloatingActionButton.js and introduced new file FABPlusIcon.js within src/components/FloatingActionButton/. These break the logic and presentation of the previous FloatingActionButton into a modular approach.

  • FABPlusIcon.js defines the Plus icon svg component for the FloatingActionButton using react-native-svg with animation based on its active state using react-native-reanimated library.
  • FloatingActionButton.js defines the new FloatingActionButton with animation and uses hooks and the react-native-reanimated library.

Fixed Issues

$ #16163
$ #16150

Tests

  1. Open the app
  2. Verify that the app icons renders correctly

FAB Animation test:

  1. Verify that the Floating action button ''+'' icon color is initially light
  2. Verify that the Floating action button ''+'' icon color is not initially rotated
  3. Verify that the Floating action button background color is initially green
  4. Click on the FAB
  5. Verify that the FAB ''+'' icon color is dark
  6. Verify that the Floating action button ''+'' icon color is rotated
  7. Verify that the Floating action button background color is dark
  8. 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 / Chrome
    • iOS / native
    • iOS / 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 a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
      • Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • 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 o[x]f 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(themeColors.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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Web
web.mp4
Mobile Web - Chrome
chrome.mp4
Mobile Web - Safari
safari.mp4
Desktop
desktop.mp4
iOS
ios.mp4
Android
android.mp4

@rayane-djouah rayane-djouah requested a review from a team as a code owner September 4, 2023 11:59
@melvin-bot melvin-bot bot requested review from rushatgabhane and removed request for a team September 4, 2023 11:59
@melvin-bot
Copy link

melvin-bot bot commented Sep 4, 2023

@rushatgabhane 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]

@rayane-djouah rayane-djouah changed the title Migrate icon/index.js to function component Migrate Icon/index.js and FloatingActionButton to function component Sep 4, 2023
@rayane-djouah
Copy link
Contributor Author

closed this PR: #25450 and opened this one to trigger the automation on both issues.

@rayane-djouah
Copy link
Contributor Author

@rushatgabhane PR is ready for review.

@rushatgabhane
Copy link
Member

Waiting for merge freeze to be over

@rayane-djouah
Copy link
Contributor Author

@rushatgabhane, I noticed that other pull requests have been approved by C+ and engineer and are now pending merge. I believe we can proceed with the review and hold the merge after the changes get approved.

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

The merge freeze is offically over! @rayane-djouah I request you to merge with the latest main and re-test at least once, thanks!

@rayane-djouah
Copy link
Contributor Author

@rushatgabhane I merged main and tested again, everything works fine.

29.New.Expensify.Mozilla.Firefox.2023-09-11.11-16-41.mp4

@rayane-djouah
Copy link
Contributor Author

ready for review

@roryabraham
Copy link
Contributor

@rushatgabhane merge freeze is lifted so feel free to review this when you're ready

Copy link
Member

@rushatgabhane rushatgabhane left a comment

Choose a reason for hiding this comment

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

@roryabraham LGTM! all platforms test well

src/components/Icon/index.tsx Outdated Show resolved Hide resolved
@roryabraham
Copy link
Contributor

Looks like some PR checks are failing now

@roryabraham roryabraham merged commit 912fa3f into Expensify:main Dec 16, 2023
15 of 17 checks passed
@OSBotify
Copy link
Contributor

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@rayane-djouah rayane-djouah deleted the Migrate-Icon/index.js-to-function-component branch December 16, 2023 22:44
@@ -148,7 +148,7 @@
"react-native-plaid-link-sdk": "10.8.0",
"react-native-qrcode-svg": "^6.2.0",
"react-native-quick-sqlite": "^8.0.0-beta.2",
"react-native-reanimated": "3.5.4",
"react-native-reanimated": "^3.6.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

Getting crash on android on latest main

android crash

Copy link
Member

Choose a reason for hiding this comment

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

weird. did you try npm ci? and rebuilding Android

Copy link
Contributor

Choose a reason for hiding this comment

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

I removed node_modules, installed npm and did fresh build

Copy link
Contributor

Choose a reason for hiding this comment

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

This also might be related

another

Copy link
Contributor

@tomekzaw tomekzaw Dec 17, 2023

Choose a reason for hiding this comment

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

@situchan Did you remember to rebuild the native app? Does npm start -- --reset-cache help?

Copy link
Contributor

Choose a reason for hiding this comment

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

Already did everything. I had to revert changes on this PR locally to unblock testing other PRs

Copy link
Contributor

@tomekzaw tomekzaw Dec 17, 2023

Choose a reason for hiding this comment

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

You're right, I can indeed reproduce this issue on 912fa3f as well. Working on it!

edit: the issue also appears if I downgrade Reanimated from 3.6.1 to 3.5.4. The error comes from fill property in FabPlusIcon.js.

Copy link
Member

Choose a reason for hiding this comment

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

@tomekzaw if possible could you please help us understand why the error comes from fill?
Is the API being used incorrectly?

https://github.com/Expensify/App/pull/26654/files#diff-58517876b1c001c5b56c9c446d55dd75c290c4e07c766fdbcb32e0d90d0d42f7R26

Copy link
Contributor

@tomekzaw tomekzaw Dec 18, 2023

Choose a reason for hiding this comment

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

@rushatgabhane I'm back with answers. Fortunately, it's not a regression in Reanimated 3.6.1 because on 3.5.4 it behaves the same way. It's also not an issue with react-native-svg (13.14.0 is okay).

If you animate fill prop of SVG elements, you need to pass adapter to useAnimatedProps, see the docs:
https://docs.swmansion.com/react-native-reanimated/docs/core/useAnimatedProps/#adapters-

You need the following adapter:

  const adapter = createAnimatedPropAdapter(
    (props) => {
      // delete props.fill;
      if (Object.keys(props).includes('fill')) {
        props.fill = { type: 0, payload: processColor(props.fill) };
      }
    },
    ['fill']
  );

Make sure to import processColor from react-native-reanimated (not react-native). Then, you pass the adapter as third argument to useAnimatedProps:

  const animatedProps = useAnimatedProps(
    () => {
      return {
        fill: interpolateColor(animatedValue.value, [0, 1], ['red', 'blue']),
      };
    },
    [],
    adapter
  );

Here's the full example for the context:

import React, { useEffect } from 'react';
import Animated, {
  Easing,
  createAnimatedPropAdapter,
  interpolateColor,
  useAnimatedProps,
  useSharedValue,
  processColor,
  withTiming,
} from 'react-native-reanimated';
import Svg, { Path } from 'react-native-svg';

const AnimatedPath = Animated.createAnimatedComponent(Path);

function FabPlusIcon() {
  const animatedValue = useSharedValue(0);

  useEffect(() => {
    animatedValue.value = withTiming(1, {
      duration: 340,
      easing: Easing.inOut(Easing.ease),
    });
  }, [animatedValue]);

  const adapter = createAnimatedPropAdapter(
    (props) => {
      // delete props.fill;
      if (Object.keys(props).includes('fill')) {
        props.fill = { type: 0, payload: processColor(props.fill) };
      }
    },
    ['fill']
  );

  const animatedProps = useAnimatedProps(
    () => {
      return {
        fill: interpolateColor(animatedValue.value, [0, 1], ['red', 'blue']),
      };
    },
    [],
    adapter
  );

  return (
    <Svg width={20} height={20}>
      <AnimatedPath
        d="M12,3c0-1.1-0.9-2-2-2C8.9,1,8,1.9,8,3v5H3c-1.1,0-2,0.9-2,2c0,1.1,0.9,2,2,2h5v5c0,1.1,0.9,2,2,2c1.1,0,2-0.9,2-2v-5h5c1.1,0,2-0.9,2-2c0-1.1-0.9-2-2-2h-5V3z"
        animatedProps={animatedProps}
      />
    </Svg>
  );
}

FabPlusIcon.displayName = 'FabPlusIcon';

export default FabPlusIcon;

cc @WoLewicki, would be good to provide some abstraction for it, e.g. useAnimatedSVGProps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I think we should also add stroke there so it looks something like this:

  const adapter = createAnimatedPropAdapter(
    (props) => {
      if (Object.keys(props).includes('fill')) {
        props.fill = { type: 0, payload: processColor(props.fill) };
      }
      if (Object.keys(props).includes('stroke')) {
        props.stroke = {type: 0, payload: processColor(props.stroke)}
      }
    },
    ['fill', 'stroke']
  );

useAnimatedSVGProps sounds good 🚀

}, [isActive, animatedValue]);

const animatedProps = useAnimatedProps(() => {
const fill = interpolateColor(animatedValue.value, [0, 1], [theme.textLight, theme.textDark]);
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI if you use foo.bar.baz inside a worklet, the whole object foo is captured and copied to the UI runtime. In some cases it's good (maybe even here, since we cache copied objects) but usually it's safer to pass the leaf nodes, e.g.:

// this is regular React code
const { baz } = foo.bar;

useAnimatedProps(() => {
  // this is a worklet
  // use `baz` directly
});

return {
transform: [{rotate: `${animatedValue.value * 135}deg`}],
backgroundColor,
borderRadius: styles.floatingActionButton.borderRadius,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same story for styles here, be aware that the whole object is copied to the UI runtime (but if it doesn't change then it's cached).


function FabPlusIcon({isActive}) {
const theme = useTheme();
const animatedValue = useSharedValue(isActive ? 1 : 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

const sharedValue

@rayane-djouah
Copy link
Contributor Author

Thank you all, I'm working on a PR now for the fix

@rayane-djouah
Copy link
Contributor Author

#33224

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.14-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/roryabraham in version: 1.4.14-0 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/jasperhuangg in version: 1.4.14-6 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 failure ❌
🕸 web 🕸 success ✅

const styles = useThemeStyles();
const iconWidth = small ? variables.iconSizeSmall : width;
const iconHeight = small ? variables.iconSizeSmall : height;
const iconStyles = [StyleUtils.getWidthAndHeightStyle(width ?? 0, height), IconWrapperStyles, styles.pAbsolute, additionalStyles];
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't we use iconWidth and iconHeight here? I think they are the correct one compared to the original file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right @tienifr, we missed this.

testID={`${this.props.src.name} Icon`}
style={this.props.additionalStyles}
testID={`${src.name} Icon`}
style={[StyleUtils.getWidthAndHeightStyle(width ?? 0, height), styles.bgTransparent, styles.overflowVisible]}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants