react-native-paper icon indicating copy to clipboard operation
react-native-paper copied to clipboard

[Feature Request] Add support for `onLongPress` on `FAB.Group` component

Open fabriziocucci opened this issue 3 years ago • 9 comments

The experience I'd like to achieve with the FAB.Group component is the following:

  • onLongPress => show the underlying FABs
  • onPress => do something else

I'm wondering, was the callback left out intentionally (e.g. to comply with the Material Design spec) or just an oversight (considering that the FAB supports onLongPress already)?

Happy to put together a PR if this is something desirable.

fabriziocucci avatar May 21 '22 17:05 fabriziocucci

Hey! Thanks for opening the issue. The issue doesn't seem to contain a link to a repro (a snack.expo.dev link or link to a GitHub repo under your username).

Can you provide a minimal repro which demonstrates the issue? A repro will help us debug the issue faster. Please try to keep the repro as small as possible and make sure that we can run it without additional setup.

github-actions[bot] avatar May 21 '22 17:05 github-actions[bot]

Couldn't find version numbers for the following packages in the issue:

  • react-native
  • react-native-paper
  • react-native-vector-icons

Can you update the issue to include version numbers for those packages? The version numbers must match the format 1.2.3.

github-actions[bot] avatar May 21 '22 17:05 github-actions[bot]

Hey @fabriziocucci, I guess there is no onLongPress functionality since MD didn't indicate that or apps following the MD don't have that functionality as well. Generally, I think we can add support for that, however opening the stack of FABs should be still handled on onPress.

lukewalczak avatar May 22 '22 10:05 lukewalczak

however opening the stack of FABs should be still handled on onPress

I guess it makes sense as default behaviour: onPress => open stack of FABs onLongPress => do nothing or open stack of FABs

Although, opening the stack of FABs even when onPress is explicitly overridden IMHO would still be somewhat inflexible.

I understand this is the current behaviour so maybe to avoid breaking changes something like this could work?

<FAB
    onPress={() => {
        // the onPress specified by the user can return true/false
        // to indicate whether the stack of FABs should open
        // (checking explicitly undefined for backward compatibility)
        const shouldToggle = onPress?.();
        if (shouldToggle === undefined || shouldToggle === true) {
            toggle();
        }
    }}
    ...

fabriziocucci avatar May 22 '22 11:05 fabriziocucci

I guess the main purpose of onPress on the FABGroup is to handle the callback on the main FAB when stack is opened.
ImoonLongPress should be an auxiliary option for handling the main FAB press, when stack is not opened.

lukewalczak avatar May 28 '22 14:05 lukewalczak

I see your point @lukewalczak, I'm wondering though what's the downside of keeping the component more flexible.

From a user point of view, a FAB and a FABGroup are initially indistinguishable, i.e. by just looking at the component in its initial state, there is no expectation in terms of interaction (for both, on press / on long press something should happen).

From a developer point of view, the FABGroup encapsulates all the logic to properly layout the stack of FABs, which I believe is the main re-use goal of the component. If I wanted to open the stack of FABs onLongPress only, I would have to copy-paste all FABGroup code in my own component, only because there is no way to prevent the default action onPress.

Sounds a bit of a limitation IMHO! :confused:

fabriziocucci avatar Jun 27 '22 08:06 fabriziocucci

I was looking for onLongPress too, but was thinking it should just do the last action in the list (like a shortcut).... so if you make the most common action, the last action in the fab group (so the one listed at the bottom/closest to the initial fab button)... I think it would be a good user experience that I can press the fab, see the menu of other fab actions, but if I just keep the initial fab button pressed, it will be the same as if I pressed the initial fab and then pressed the lowest fab action in the menu list. So just a shortcut of 1 long press instead of 2 normal presses for the most common action in the list.

dkhunt27 avatar Jul 26 '22 20:07 dkhunt27

This is another example of a (very) niche use case which would be covered by the implementation proposal I suggested (i.e. allow free customization of onLongPress) but wouldn't be possible with the one you suggested @lukewalczak.

fabriziocucci avatar Jul 27 '22 08:07 fabriziocucci

Could you please provide some app examples when there is a long press functionality on the FAB button? We are still discussing that feature and direction in which to go.

lukewalczak avatar Jul 28 '22 12:07 lukewalczak

twitter does this, on press opens a new tweet, while long press gives you opens the fab group

also the fab currently accepts a onLongPress prop, if Fab group would just pass it down it would be easy to make it work with current functionality

jgarciach avatar Oct 13 '22 18:10 jgarciach

In general, I'm all for adding support for an onLongPress functionality in the FAB component, however, there are some principles:

  1. we should not interfere with the default behaviour of opening the fab stack on FAB press. That behaviour is indicated in the MD guidelines and components in the react-native-paper library by default are following that rules.

  2. onLongPress should be an auxiliary option passed into the FAB and its functionality should be individually handled by the user – by default onLongPress shouldn't have any predefined behavior.

lukewalczak avatar Oct 27 '22 14:10 lukewalczak

  1. we should not interfere with the default behaviour of opening the fab stack on FAB press. That behaviour is indicated in the MD guidelines and components in the react-native-paper library by default are following that rules.

I agree but not changing the default behavior is one thing, not having the flexibility to override the default behavior is another. Are we saying that there will be no way to customize the default onPress behavior for FAB.Group?

fabriziocucci avatar Oct 27 '22 14:10 fabriziocucci

I agree but not changing the default behavior is one thing, not having the flexibility to override the default behavior is another. Are we saying that there will be no way to customize the default onPress behavior for FAB.Group?

We can try to make it flexible, but a new solution cannot break the default behaviour.

lukewalczak avatar Oct 27 '22 15:10 lukewalczak

The solution I suggested intially was to replace this with something like:

<FAB
    onPress={() => {
        // the onPress specified by the user can return true/false
        // to indicate whether the stack of FABs should open
        // (checking explicitly undefined for backward compatibility)
        const shouldToggle = onPress?.();
        if (shouldToggle === undefined || shouldToggle === true) {
            toggle();
        }
    }}
    ...

The obvious alternative is to add an additional prop to FABGroup (e.g. disableToggleOnPress).

Any other option comes up to mind @lukewalczak?

fabriziocucci avatar Oct 27 '22 15:10 fabriziocucci

I think introducing the new prop e.g. toggleStackOnPressEnabled with default true value is more convenient.

lukewalczak avatar Oct 27 '22 20:10 lukewalczak

@lukewalczak I could give this a try unless you guys want to handle this internally.

fabriziocucci avatar Oct 30 '22 09:10 fabriziocucci

Sure @fabriziocucci, I will be happy to accept your contribution!

lukewalczak avatar Oct 30 '22 10:10 lukewalczak