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

MenuItem icon should be conditionally rendered #71

Closed
EvanBacon opened this issue Jun 20, 2022 · 12 comments
Closed

MenuItem icon should be conditionally rendered #71

EvanBacon opened this issue Jun 20, 2022 · 12 comments
Labels
bug Something isn't working wip Work in progress

Comments

@EvanBacon
Copy link

EvanBacon commented Jun 20, 2022

Describe the bug

In the expo example, the icons do not render for a couple reasons:

  1. The iconProvider is not added.
  2. The icons are defined as functions instead of strings, I fixed this locally by patching this line:
{!item.isTitle && item.icon && AnimatedIcon && (
  typeof item.icon === 'string' ? <AnimatedIcon name={item.icon} size={18} style={textColor} /> : item.icon()
)}

Notice I also conditionally hide the icons if they aren't defined, this isn't great but it does prevent the 'rendering undefined' bug.

Here is the diff that solved my problem:

diff --git a/node_modules/react-native-hold-menu/src/components/menu/MenuItem.tsx b/node_modules/react-native-hold-menu/src/components/menu/MenuItem.tsx
index efa3e73..18674cd 100644
--- a/node_modules/react-native-hold-menu/src/components/menu/MenuItem.tsx
+++ b/node_modules/react-native-hold-menu/src/components/menu/MenuItem.tsx
@@ -64,8 +64,8 @@ const MenuItemComponent = ({ item, isLast }: MenuItemComponentProps) => {
         >
           {item.text}
         </Animated.Text>
-        {!item.isTitle && item.icon && (
-          <AnimatedIcon name={item.icon} size={18} style={textColor} />
+        {!item.isTitle && item.icon && AnimatedIcon && (
+          typeof item.icon === 'string' ? <AnimatedIcon name={item.icon} size={18} style={textColor} /> : item.icon()
         )}
       </AnimatedTouchable>
       {item.withSeparator && <Separator />}
diff --git a/node_modules/react-native-hold-menu/src/components/menu/MenuList.tsx b/node_modules/react-native-hold-menu/src/components/menu/MenuList.tsx
index c476f1d..fb02325 100644
--- a/node_modules/react-native-hold-menu/src/components/menu/MenuList.tsx
+++ b/node_modules/react-native-hold-menu/src/components/menu/MenuList.tsx
@@ -33,7 +33,7 @@ import { useInternal } from '../../hooks';
 import { deepEqual } from '../../utils/validations';
 import { leftOrRight } from './calculations';
 
-const MenuContainerComponent = IS_IOS ? BlurView : View;
+const MenuContainerComponent = BlurView;
 const AnimatedView = Animated.createAnimatedComponent<{
   animatedProps: Partial<{ blurType: string }>;
 }>(MenuContainerComponent);
@@ -72,8 +72,8 @@ const MenuListComponent = () => {
       state.value === CONTEXT_MENU_STATE.ACTIVE
         ? withSpring(1, SPRING_CONFIGURATION_MENU)
         : withTiming(0, {
-            duration: HOLD_ITEM_TRANSFORM_DURATION,
-          });
+          duration: HOLD_ITEM_TRANSFORM_DURATION,
+        });
 
     const opacityAnimation = () =>
       withTiming(state.value === CONTEXT_MENU_STATE.ACTIVE ? 1 : 0, {
@@ -104,8 +104,8 @@ const MenuListComponent = () => {
             ? 'rgba(255, 255, 255, .75)'
             : 'rgba(255, 255, 255, .95)'
           : IS_IOS
-          ? 'rgba(0,0,0,0.5)'
-          : 'rgba(39, 39, 39, .8)',
+            ? 'rgba(0,0,0,0.5)'
+            : 'rgba(39, 39, 39, .8)',
     };
   }, [theme]);
 
@@ -114,6 +114,7 @@ const MenuListComponent = () => {
   }, [theme]);
 
   const setter = (items: MenuItemProps[]) => {
+    console.log('add items:', items)
     setItemList(items);
     prevList.value = items;
   };

This issue body was partially generated by patch-package.

Notice dropping the IS_IOS check fixed the blur issue as well.

@EvanBacon EvanBacon added the bug Something isn't working label Jun 20, 2022
@SohelIslamImran
Copy link

Facing the same issue. Can't use icon: () => <Icon name="delete" size={18} /> as mentioned in this example https://enesozturk.github.io/react-native-hold-menu/docs/examples#list-with-icons

@SohelIslamImran
Copy link

Can't you create an RP? @EvanBacon

@SohelIslamImran
Copy link

Please fix this issue ASAP @enesozturk

@SohelIslamImran
Copy link

Also, How can I change the Backdrop background color?

@enesozturk
Copy link
Owner

Hey guys, so sorry for being late, cannot check the issue of the hold menu for a long time due to my personal occupation. @EvanBacon thanks a lot for the issue and fix 🫶🏼
@SohelIslamImran I'll take a look in a day and puvlish new version.

@SohelIslamImran
Copy link

Hey guys, so sorry for being late, cannot check the issue of the hold menu for a long time due to my personal occupation. @EvanBacon thanks a lot for the issue and fix 🫶🏼 @SohelIslamImran I'll take a look in a day and puvlish new version.

Thank you so much

@SohelIslamImran
Copy link

@enesozturk Also, Please try to make this package more customizable (styles, state, variables, props, etc)

@SohelIslamImran
Copy link

It doesn't have blurry background (Backdrop) on IOS as shown in the examples pictures @enesozturk

@SohelIslamImran
Copy link

Hey guys, so sorry for being late, cannot check the issue of the hold menu for a long time due to my personal occupation. @EvanBacon thanks a lot for the issue and fix 🫶🏼 @SohelIslamImran I'll take a look in a day and puvlish new version.

Please fix them as you said ASAP. We are working on a production project, we need to use it if all these issues are resolved.

@enesozturk
Copy link
Owner

Yeah, today I was working on hold menu and realized it, there are also some other issues I need to solve, blur background looks related to dependencies. I'll handle it.

@enesozturk
Copy link
Owner

Published a new release. The icon rendering issue should be fixed now. Along with the same PR, the following issues should be fixed:

  • Android backdrop gesture handler was not working
  • Icon colors were not changing as the theme change
  • Blur view was not working on iOS

Awesome, lets goo 💯

@danieloi
Copy link

Hey guys, so sorry for being late, cannot check the issue of the hold menu for a long time due to my personal occupation. @EvanBacon thanks a lot for the issue and fix 🫶🏼 @SohelIslamImran I'll take a look in a day and puvlish new version.

Please fix them as you said ASAP. We are working on a production project, we need to use it if all these issues are resolved.

Hey guys, so sorry for being late, cannot check the issue of the hold menu for a long time due to my personal occupation. @EvanBacon thanks a lot for the issue and fix 🫶🏼 @SohelIslamImran I'll take a look in a day and puvlish new version.

Please fix them as you said ASAP. We are working on a production project, we need to use it if all these issues are resolved.

Unless you're contributing significantly financially to an open source project, to use words like ASAP the way you're doing here in text format comes off as very unpleasant.

Saw you use the same tone in other issues.

Take it easy buddy

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working wip Work in progress
Projects
None yet
Development

No branches or pull requests

4 participants