chore(exoflex): update `react`, `react-native` & `react-native-paper` version #713

StefanusChristian · 2023-01-25T08:15:15Z

Changes:

Package & Example app changes:

Upgrade react, react-native, react-native-web, react-native-paper, react-native-collapsible version.
Change react-native-calendars from Yoan repository into latest version. (changes from @JonathanRadotski https://github.com/StefanusChristian/infra/pull/1/files)
Add new expo project in example directory. Since the previous one was still in SDK40, it is easier to create a new one instead of updating it to SDK47.
Add patch-package to both exoflex (patch react-native-collapsible) & exoflex/new-example (patch react-navigation-drawer).
- react-native-collapsible: Adding the fix that has been merged to the package but not released yet. commit
- react-navigation-drawer: This package is deprecated now and have been included into react-navigation instead. But I think it was introduced in version 5.x.x (docs) and we still use 4.x.x (docs). So for a quick fix, I follow this guide for the error: TypeError: interpolate is not a function react-navigation/react-navigation#9665 (comment)

Adjusted Component:

Accordion: Add useNativeRiver props to useAnimation()
Badge: Add textStyle typing
Button:
- ButtonOpacity:
  - Update ref as RefObject<TouchableOpacity> since ButtonProp types based on react-native-paper types and the ref was RefObject<View> (from Surface component).
  - Adjust usage of accessibilityTraits & accessibilityComponentType into accessibilityStates. (Reference)
  - Adjust Icon props naming from color to iconColor.
- ButtonRipple:
  - Adjust usage of accessibilityTraits & accessibilityComponentType into accessibilityStates.
  - Adjust Icon props naming from color to iconColor.
Calendar: Adjust CalendarProps to follow latest version of the package.
Chip: Adjust Icon props naming from color to iconColor.
Collapsible: Add useNativeRiver props to useAnimation()
DateTimePicker: Adjust DateTimePicker.web type from DateObject into DateData.
Menu: Adjust PaperMenu.Item props naming from icon to leadingIcon.
SegmentedControl: Add StyleProp<ViewStyle> in both Indicator & SegmentedControl.
Switch: Add StyleProp<ViewStyle> types.
Toast: Adjust Icon props naming from color to iconColor.

Contants, Helper & Types Changes:

Theme: Add 2 new color key as part of the Theme: notification & tooltip.
getPaperTheme: Adjust import naming from Theme into MD2Theme and adjust the props needed.
Colors types: Add new key tooltip as Colors type.
Custom react-native-color-slider types: Change number[] into Array<number>

How to test:

Move to packages/exoflex and run yarn install to install dependencies.
Move to packages/exoflex/example and also run yarn install. Once it's done, run yarn start to test the component.

Notes:

Based on testing, the only component that has problem is ButtonRipple. The button didn't work properly on iOS simulator, but animation run properly on Android (cc: @JonathanRadotski @Udayawibawamukti).

…le` package

[Chores][Fix] Update RN calendars

…ts-error [Chores][Fix] Update RN calendars

…awer`

ikusa

Thanks for upgrading all of these. The changes looks good but I've several suggestion and question. We also might need @darcien help to review these too, since I'm not super familiar with exoflex

ikusa · 2023-01-30T08:12:20Z

packages/exoflex/package.json

    "react-native-animation-hooks": "^1.0.1",
-    "react-native-calendars": "https://github.com/oshimayoan/react-native-calendars/archive/oshimayoan-0.0.3.tar.gz",
-    "react-native-collapsible": "^1.5.1",
+    "react-native-calendars": "^1.1293.0",


On reverting this to the newest feat, have we test these change support what Yoan originally patch out?

I think it's already supported on the newest version, but let me include @JonathanRadotski to the discussion. What do you think?

Briefly checked and comparing added feature. I think the newest version is already supported with added feature from previous package and good to go 👍.

packages/exoflex/example/metro.config.js

ikusa · 2023-01-30T08:44:50Z

packages/exoflex/example/patches/react-navigation-drawer+2.7.2.patch

+diff --git a/node_modules/react-navigation-drawer/lib/module/views/Drawer.js b/node_modules/react-navigation-drawer/lib/module/views/Drawer.js
+index 3f81450..cf976b7 100644
+--- a/node_modules/react-navigation-drawer/lib/module/views/Drawer.js
+++ b/node_modules/react-navigation-drawer/lib/module/views/Drawer.js


After checking the issue, from my understanding it seems this was caused by we using react-navigation 4. It's not required to do this now but I think it's good to also upgrade it to react-navigation 5/6.

If the upgrade is not a 5 mins job, lets just land this first and upgrade on separate PR

Got it, will try and report later.

Based on this: https://reactnavigation.org/docs/5.x/upgrading-from-4.x#navigation-container

I thinks it's better to upgrade on separate PR, since we still using createAppContainer to wrap our RootNavigator.

ikusa · 2023-01-30T08:49:08Z

packages/exoflex/src/components/Accordion.tsx

-        : !!renderIconRight
-        ? renderIconRight(animatedValue)
-        : DefaultIcon}
+      <>


I've checked out these changes and it seems now will throw error when we use more than one child. Is this supposed to work like that? or is it possible we messed up with something? React native doc say View is designed to be nested inside other views and can have 0 to many children of any type.

Might be something from the collapsible package we use which attach a gesture handler to the children, requiring specifically only 1 child. Need investigation as I don't remember the detail here.

packages/exoflex/tsconfig.json

ikusa · 2023-01-30T09:00:44Z

@StefanusChristian Also do we have the example of buttonRipple not working properly on the IOS? curious what's the actual result of these bug

packages/exoflex/example/tsconfig.json

darcien · 2023-01-30T09:57:29Z

packages/exoflex/example/patches/react-navigation-drawer+2.7.2.patch

+diff --git a/node_modules/react-navigation-drawer/lib/module/views/Drawer.js b/node_modules/react-navigation-drawer/lib/module/views/Drawer.js
+index 3f81450..cf976b7 100644
+--- a/node_modules/react-navigation-drawer/lib/module/views/Drawer.js
+++ b/node_modules/react-navigation-drawer/lib/module/views/Drawer.js


If the upgrade is not a 5 mins job, lets just land this first and upgrade on separate PR

darcien · 2023-01-30T10:02:38Z

packages/exoflex/src/components/Button/ButtonOpacity.tsx

@@ -38,19 +39,18 @@ export default function ButtonOpacity(props: ButtonProps) {
  return (
    <TouchableOpacity
      {...otherProps}
+      ref={ref as RefObject<TouchableOpacity>}


I'm not sure if passing ref directly will work or not.
This will need a bit of testing and see if this needs use forwardRef instead.

For now I think this is OK, just make sure to not release this before testing whether the ref works or not.

Got it, will test it.

@ikusa @darcien

About this problem, I think I'll open up a discussion in here. So the initial reason why I add this change was because the parent exoflex Button component has props with type ButtonProps which inherit PaperButton types.

PaperButton props type also inherit PaperSurface type, which has the ref prop with type React.RefObject<View>

Our exoflex Button has 2 type: ButtonOpacity & ButtonRipple, which will rendered based on useRipple props.

With ButtonRipple, which will render TouchableRipple from react-native-paper, it seems the passed <React.RefObject<View>> ref from ButtonProps didn't raise any type error. Turns out TouchableRipple inherits its prop type from RN Pressable and also has the forwardedRef - React.RefAttributes<View>.

But with ButtonOpacity, which will render TouchableOpacity from react-native, it raises ref type error. Because the desired type was LegacyRef<TouchableOpacity> but supplied with RefObject<View>. That's why I passing the ref like that and typecast it as RefObject<TouchableOpacity> to fulfilled the required type.

As for using the forwardRef, the same problem arise when I need to type the ref from using forwardRef on Parent Button component.

Maybe you guys can enlighten me on how to solve this problem?

It's been a while since I worked on this area, so it might be a bit hard to give exact code solution.

From a glance, we should be able to resolve this type conflict by migrating away from TouchableOpacity and use Pressable instead. That way the ref is always a View ref.

The cons: we need to mimic the old TouchableOpacity using Pressable to keep the same behavior. Unless we want to change the default button behavior, and just do a breaking changes.

@ikusa @darcien

Got it! Changes on using Pressable are in this commit: e46eaf3

I added animated opacity to make the feedback looks like TouchableOpacity and it seems to work as intended.

Pressable.Opacity.mp4

Looks great 👍

You might want to put the animated value in a ref and memoize the fade transition functions for optimizations.

Also, probably it's not a good idea to override the onPressIn/onPressOut from other props with our fade animation. Will need to extend it so we also call the onPressIn/onPressOut from the props.

Got it! Applied in here: 4971cb9

packages/exoflex/src/components/Badge.tsx

darcien · 2023-01-30T10:05:49Z

packages/exoflex/src/components/Accordion.tsx

-        : !!renderIconRight
-        ? renderIconRight(animatedValue)
-        : DefaultIcon}
+      <>


Might be something from the collapsible package we use which attach a gesture handler to the children, requiring specifically only 1 child. Need investigation as I don't remember the detail here.

packages/exoflex/example/webpack.config.js

darcien · 2023-01-30T10:10:35Z

Great work @StefanusChristian! We might need to update the repo settings before merging your changes because right now it's blocked by a dead circleci check.

StefanusChristian · 2023-02-01T05:36:59Z

@ikusa @darcien
Here's the video showing the ButtonRipple problem on iOS. The button onPress is invoked but didn't show the ripple animation at all.

Button.Ripple.Problem.mp4

darcien · 2023-02-01T06:36:58Z

Let's not block the upgrade PR on the button ripple issue. I'm keen to ship this B I G P R ASAP and address the minor issues separately.

darcien

note on the memo dependency issue

darcien · 2023-02-03T12:23:47Z

packages/exoflex/src/components/Button/ButtonOpacity.tsx

+    // eslint-disable-next-line react-hooks/exhaustive-deps
+  }, []);


Let's not disable this lint rule and fix the warning.

Not setting the dependency for useCallback correctly might cause subtle bugs with stale values.

Actually, let's do optimization separately now that I think about it. See my next comment.

darcien · 2023-02-03T12:28:57Z

packages/exoflex/src/components/Button/ButtonOpacity.tsx

+      onPressIn={(e) => {
+        onPressIn && onPressIn(e);
+        fadeOpacityIn();
+      }}


Here, we create another arrow function inline, which is the thing we want to optimize before by moving the arrow function to useCallback.

If you want a deep dive on this, beta docs from React is a good starting point, like: https://beta.reactjs.org/reference/react/useCallback#should-you-add-usecallback-everywhere

But in the meantime, let's just keep the fix in and iterate on the optimization later.

Got it, will do.
For the repo setting (remove/bypass the circleci check), do you have the authority for that @darcien? I think I don't have the permission, so I can't adjust the setting

darcien · 2023-02-06T04:09:27Z

For the repo setting (remove/bypass the circleci check), do you have the authority for that @darcien? I think I don't have the permission, so I can't adjust the setting

I've removed the required CircleCI check for merging the PR.
In the future, we will need to migrate the CI.

@StefanusChristian can you try running the tests locally and see if it passes?
After that is checked, we should be good to go

StefanusChristian · 2023-02-07T04:11:12Z

For the repo setting (remove/bypass the circleci check), do you have the authority for that @darcien? I think I don't have the permission, so I can't adjust the setting

I've removed the required CircleCI check for merging the PR. In the future, we will need to migrate the CI.

@StefanusChristian can you try running the tests locally and see if it passes? After that is checked, we should be good to go

It seems the jest preset is error for getBaseWebPreset, since the jest-preset is removed from the latest react-native-web (0.18.xx). Reference: necolas/react-native-web#2312

Do you guys have any idea on how to fix the jest-preset? @ikusa @darcien

StefanusChristian · 2023-02-10T10:02:31Z

@ikusa @darcien
Web jest & test finally adjusted 🎉🎉🎉

Passed Tests

Passed.exoflex.test.mp4

I'll list the changes for making the web test work in here:

Adjust the getBaseWebPreset (web platform jest-preset) to use module mapper based on latest rnw docs and set testEnvironment to jsdom for web.
Upgrade @testing-library/react & react-dom to match our current react version.
Add testID on some component that will turn into data-testid when rendered on our web test. Reference.
Adjust function from @testing-library/react: wait into waitFor and fireEvent.click usage. For fireEvent adjustment, somehow the fireEvent.click call doesn't trigger the mock function, so I use below form instead and it works. This might need more time to investigate but I think the adjustment is good for now

From:

fireEvent.click(getByTestId('button'));

Into:

fireEvent(
  getByTestId('button'),
  new MouseEvent('click', {
    bubbles: true,
    cancelable: true,
  }),
);

ikusa

Let's merge this

StefanusChristian added the wip Work in progress label Jan 25, 2023

StefanusChristian added 9 commits January 25, 2023 15:44

Upgrade react, react-native and react-native-paper version

940f534

Adjust useAnimation and changed props name

50f2f7a

Adjust types changes

d161755

Upgrade exoflex/example expo to version SDK41

a175477

Upgrade exoflex/example expo to version SDK42

cab9361

Upgrade exoflex/example expo to version SDK43

a1f013b

Upgrade exoflex/example expo to version SDK44

e536d7a

New Example init

5ac1c22

Update & adjust: react-native-multi-slide & `react-native-collapsib…

538739c

…le` package

StefanusChristian force-pushed the exoflex/accessibilityTraits-error branch from e1722b9 to 538739c Compare January 27, 2023 06:18

JonathanRadotski and others added 7 commits January 27, 2023 13:50

update react-native-calendars

1038750

resolve conflict package json

98fc05d

Merge pull request #1 from JonathanRadotski/update-RNcalendars

05c0903

[Chores][Fix] Update RN calendars

Merge pull request #1 from JonathanRadotski/exoflex/accessibilityTrai…

9113069

…ts-error [Chores][Fix] Update RN calendars

Revert unnecessary changes and fix double # on color

0823a0b

new-example: Add patch-package& use latest react-native-calendars

4800126

new-example: Add reanimated babel config & patch `react-navigation-dr…

a53153a

…awer`

StefanusChristian marked this pull request as ready for review January 27, 2023 09:02

StefanusChristian added ready for review and removed wip Work in progress labels Jan 30, 2023

Replace example project with new expo project

69a8904

StefanusChristian force-pushed the exoflex/accessibilityTraits-error branch from aa66095 to 69a8904 Compare January 30, 2023 08:20

Remove unused assets

5411ed4

ikusa previously approved these changes Jan 30, 2023

View reviewed changes

darcien reviewed Jan 30, 2023

View reviewed changes

Fix some nit

0b26bd1

StefanusChristian dismissed ikusa’s stale review via 0b26bd1 February 1, 2023 11:04

StefanusChristian added 2 commits February 3, 2023 14:54

Replace TouchableOpacity with Pressable + AnimatedOpacity

e46eaf3

Apply optimization suggestion

4971cb9

darcien reviewed Feb 3, 2023

View reviewed changes

StefanusChristian added 5 commits February 9, 2023 15:39

Adjust jest config for iOS & Android, disable web test

722931d

Update jest preset for web testing

3302505

Upgrade @testing-library/react & react-dom package

98f7814

Adding testID prop for web testing purposes

108f0b7

Adjust web test cases

73af2e1

Upgrade circleci node docker image version

ea33f2c

ikusa approved these changes Feb 20, 2023

View reviewed changes

ikusa merged commit 173a951 into kodefox:master Feb 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(exoflex): update `react`, `react-native` & `react-native-paper` version #713

chore(exoflex): update `react`, `react-native` & `react-native-paper` version #713

StefanusChristian commented Jan 25, 2023 •

edited

Loading

ikusa left a comment

ikusa Jan 30, 2023

StefanusChristian Feb 1, 2023

JonathanRadotski Feb 1, 2023 •

edited

Loading

ikusa Jan 30, 2023

darcien Jan 30, 2023

StefanusChristian Feb 1, 2023

StefanusChristian Feb 1, 2023

ikusa Jan 30, 2023

darcien Jan 30, 2023

ikusa commented Jan 30, 2023

darcien Jan 30, 2023

darcien Jan 30, 2023

StefanusChristian Feb 1, 2023

StefanusChristian Feb 1, 2023 •

edited

Loading

darcien Feb 1, 2023

StefanusChristian Feb 3, 2023

darcien Feb 3, 2023 •

edited

Loading

StefanusChristian Feb 3, 2023

darcien Jan 30, 2023

darcien commented Jan 30, 2023

StefanusChristian commented Feb 1, 2023

darcien commented Feb 1, 2023

darcien left a comment

darcien Feb 3, 2023

darcien Feb 3, 2023

StefanusChristian Feb 6, 2023

darcien commented Feb 6, 2023 •

edited

Loading

StefanusChristian commented Feb 7, 2023

StefanusChristian commented Feb 10, 2023 •

edited

Loading

ikusa left a comment

		// eslint-disable-next-line react-hooks/exhaustive-deps
		}, []);

chore(exoflex): update react, react-native & react-native-paper version #713

chore(exoflex): update react, react-native & react-native-paper version #713

Conversation

StefanusChristian commented Jan 25, 2023 • edited Loading

Changes:

How to test:

Notes:

ikusa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonathanRadotski Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikusa commented Jan 30, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StefanusChristian Feb 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darcien Feb 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darcien commented Jan 30, 2023

StefanusChristian commented Feb 1, 2023

darcien commented Feb 1, 2023

darcien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darcien commented Feb 6, 2023 • edited Loading

StefanusChristian commented Feb 7, 2023

StefanusChristian commented Feb 10, 2023 • edited Loading

ikusa left a comment

Choose a reason for hiding this comment

chore(exoflex): update `react`, `react-native` & `react-native-paper` version #713

chore(exoflex): update `react`, `react-native` & `react-native-paper` version #713

StefanusChristian commented Jan 25, 2023 •

edited

Loading

JonathanRadotski Feb 1, 2023 •

edited

Loading

StefanusChristian Feb 1, 2023 •

edited

Loading

darcien Feb 3, 2023 •

edited

Loading

darcien commented Feb 6, 2023 •

edited

Loading

StefanusChristian commented Feb 10, 2023 •

edited

Loading