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.

Sign up for GitHub

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

[CP Stag] Only use tab animation when in tab navigator #27974

Merged
merged 3 commits into from
Sep 22, 2023
Merged

[CP Stag] Only use tab animation when in tab navigator #27974

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6401db3
Only use tab animation when in tab navigator
Sep 21, 2023
827bed4
Separate components and add isInTabNavigator prop
Sep 22, 2023
bcc5bd4
Add proptype for nonnative component
Sep 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions src/pages/iou/ReceiptSelector/NavigationAwareCamera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,19 @@ function NavigationAwareCamera({cameraTabIndex, forwardedRef, ...props}) {
const navigation = useNavigation();
const [isCameraActive, setIsCameraActive] = useState(navigation.isFocused());

const isInTabNavigator = cameraTabIndex > -1;
Copy link
Member

Choose a reason for hiding this comment

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

This is a workaround to detect if we are in a tab.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, but I don't know I prefer isTab or something similar to proposal 2. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Even if we use proposal 2 is comes down to conditional use of hook.

// Get the animation value from the tab navigator. Its a value between 0 and the
// number of pages we render in the tab navigator. When we even just slightly start to scroll to the camera page,
// (value is e.g. 0.001 on animation start) we want to activate the camera, so its as fast as possible active.
const tabPositionAnimation = useTabAnimation();
// We can use a condition to call a hook here because otherwise the hook throws
// eslint-disable-next-line react-hooks/rules-of-hooks
const tabPositionAnimation = isInTabNavigator ? useTabAnimation() : null;
Copy link
Member

@parasharrajat parasharrajat Sep 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

while this may be working, it is not the correct way of using hooks. An alternative would be to create two screens. One for direct access and one for in-tab selector. Define these in routes.

  1. Now create a common component from the screen logic.
  2. use this component on both screens.
  3. pass prop to manage if we want to use navigationAwareCamera with TabNavigation or a similar component without tab navigation.
  4. We will create two components for navigationAwareCamera, one with tabAnimation hook and the other without it. These are used in step 3.
  5. For the Replace button, we will navigate to the direct screen. Not in the tab selector.
  1. We can also update the patch to skip throwing errors and just issue a console warning.
  2. But this is more error-prone as users might use it in the wrong place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would prefer the second option. Creating that much boilerplate just for not calling a hook conditionally isn't worth it IMO.

Copy link
Member

Choose a reason for hiding this comment

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

So, I checked and it seems 1-2 are already done in the code. We only need to do 3-4.

Copy link
Member

Choose a reason for hiding this comment

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

The main component used is ReceiptSelector. So we can create a prop on it saying hasTab or something and then just switch between the two versions of navigationAwareCamera component.

Copy link
Member

Choose a reason for hiding this comment

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

I won't recommend the second option has it will lead to the same issue which we prevented by throwing the error. We want to prevent he user from using this hook in non tab screens.


useEffect(() => {
if (!isInTabNavigator) {
Copy link
Member

Choose a reason for hiding this comment

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

We skipped this hook, did you check if the camera is opening?

I think we will have to add a different hook to activate the camera when not inside Tab Selector.

Copy link
Member

Choose a reason for hiding this comment

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

Something like https://reactnavigation.org/docs/stack-navigator/#transitionstart

return;
}

const listenerId = tabPositionAnimation.addListener(({value}) => {
// Activate camera as soon the index is animating towards the `cameraTabIndex`
setIsCameraActive(value > cameraTabIndex - 1 && value < cameraTabIndex + 1);
Expand All @@ -33,7 +40,7 @@ function NavigationAwareCamera({cameraTabIndex, forwardedRef, ...props}) {
return () => {
tabPositionAnimation.removeListener(listenerId);
};
}, [cameraTabIndex, tabPositionAnimation]);
}, [cameraTabIndex, isInTabNavigator, tabPositionAnimation]);

// Note: The useEffect can be removed once VisionCamera V3 is used.
// Its only needed for android, because there is a native cameraX android bug. With out this flow would break the camera:
Expand Down
2 changes: 1 addition & 1 deletion src/pages/iou/ReceiptSelector/index.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ function ReceiptSelector({route, report, iou, transactionID}) {

const iouType = lodashGet(route, 'params.iouType', '');
const reportID = lodashGet(route, 'params.reportID', '');
const pageIndex = lodashGet(route, 'params.pageIndex', 1);
const pageIndex = lodashGet(route, 'params.pageIndex', -1);

const {translate} = useLocalize();

Expand Down