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

Long click action for chips #1822

Closed
Fra078 opened this issue Nov 19, 2023 · 18 comments
Closed

Long click action for chips #1822

Fra078 opened this issue Nov 19, 2023 · 18 comments
Assignees
Labels
enhancement New feature or request

Comments

@Fra078
Copy link

Fra078 commented Nov 19, 2023

It's a common task to make a chip that when long clicked do a secondary action so why not introduce a onLongClick parameter for material Chips which implicitly use the combined clickable modifier.

@yschimke
Copy link
Collaborator

Are you interested in submitting a PR?

@Fra078
Copy link
Author

Fra078 commented Nov 20, 2023

What is?

@yschimke
Copy link
Collaborator

It's a good feature request. I'm just checking if you are asking for it to be implemented, or if you are interested in contributing it by submitting a PR.

@Fra078
Copy link
Author

Fra078 commented Nov 20, 2023

I just wanted to recommend a feature, unfortunately I don't have enough time to contribute

@yschimke yschimke added the enhancement New feature or request label Nov 20, 2023
@MohitMandalia
Copy link
Contributor

@yschimke Do we also want to inclide onDoubleClick??

Just to be clear we will now use combinedClickable for all of these

@luizgrp
Copy link
Member

luizgrp commented Jan 1, 2024

hey @Kpeved, do you have any thoughts on what would the best approach to implement this?

@Kpeved Kpeved self-assigned this Jan 3, 2024
@Kpeved
Copy link
Collaborator

Kpeved commented Jan 3, 2024

Checking whether it's possible to have a workaround for a current implementation. If not - we might consider a new api for Chips. ( in M3 and possibly in M2.5)
I think the same problem might exist for other components - such as Button

@Kpeved
Copy link
Collaborator

Kpeved commented Jan 8, 2024

To summarize my findings - it's not possible to add now another clickable outside of the component ( eg extend Chip or Button externally and add other callbacks like LongClick) - for that we have to rewrite the component itself ( by using combinedClickable or other modifiers) .
This not only applies to Wear Compose and Horologist, but to the core Compose components (phone/desktop) as well.

The only way to achieve this now will be to copy existing implementations and rewrite clickable handling there

@yschimke
Copy link
Collaborator

yschimke commented Jan 8, 2024

Do we want to add those copies here to support LongClick? Or could this be added in Wear Compose without changing the API, just modifiers? I'm assuming not.

@Kpeved
Copy link
Collaborator

Kpeved commented Jan 8, 2024

Theoretically we can add them in Wear Compose, but that's not a quick solution.

@yschimke
Copy link
Collaborator

@Kpeved would this be possible by wrapping the Wear Compose button with a simple Box, that has the clickable, but sharing the interactionSource?

If so, we could put that in Horologist and supporting the combinedClickable properties as params.

@IsakTheHacker
Copy link

@yschimke I implemented your idea, but instead of wrapping the Wear Compose button with a Box, I did the exact opposite. Here is my final solution:

@OptIn(ExperimentalFoundationApi::class)
@Composable
internal fun MultiClickButton(
	onClick: () -> Unit,
	modifier: Modifier = Modifier,
	onLongClick: (() -> Unit)? = null,
	onDoubleClick: (() -> Unit)? = null,
	enabled: Boolean = true,
	colors: ButtonColors = ButtonDefaults.primaryButtonColors(),
	interactionSource: MutableInteractionSource = remember { MutableInteractionSource() },
	shape: Shape = CircleShape,
	border: ButtonBorder = ButtonDefaults.buttonBorder(),
	content: @Composable BoxScope.() -> Unit,
) {
	Button(
		onClick = {},
		enabled = enabled,
		colors = colors,
		interactionSource = interactionSource,
		shape = shape,
		border = border,
	) {
		Box(
			modifier = modifier
				.combinedClickable(
					interactionSource = interactionSource,
					indication = null,		//We already get the visual indication by using the standard Material Button
					enabled = enabled,
					onClick = onClick,
					onLongClick = onLongClick,
					onDoubleClick = onDoubleClick,
				),
			contentAlignment = Alignment.Center,
			content = content,
		)
	}
}

I also tried having the Box be the outer composable and the Button as content in the BoxScope, but that resulted in the same behaviour I described in #1979. Maybe something similar to this solution could be added in Horologist?

@yschimke
Copy link
Collaborator

Nice

Is the touch target of the box the same size as
the button.

@IsakTheHacker
Copy link

Yes, it only receives touch inputs that are within the circle covered by the button

@Kpeved
Copy link
Collaborator

Kpeved commented Jan 16, 2024

Thanks for proposal @IsakTheHacker. That's great suggestion, it works!

But be aware that it handles clicks only for a section which your content takes. For Button it's ok, because we don't have any paddings in it. But we have inner paddings in some components - like Chip or Card. So you have to override it internally - removing it from Chip or Card, and adding it to the inner Box .
Something like this

... 
    Chip(
        onClick = {},
        contentPadding = PaddingValues(0.dp),
        enabled = enabled,
        interactionSource = interactionSource,
        shape = shape,
    ) {
        Box(
            modifier = modifier
                .fillMaxSize()
                .combinedClickable(
                    interactionSource = interactionSource,
                    indication = null,        //We already get the visual indication by using the standard Material Button
                    enabled = enabled,
                    onClick = onClick,
                    onLongClick = onLongClick,
                    onDoubleClick = onDoubleClick,
                )
                .padding(ChipDefaults.ContentPadding) // padding should go after specified clickable

A real bummer is that in androidx.compose Chip has a hardcoded padding, which can't be overriden :/ . But in wear components we have this option :)

@yschimke
Copy link
Collaborator

yschimke commented Feb 2, 2024

There is some fillMaxSize when we try with Chip, but an example with Button and Card #2021 (review)

@yschimke
Copy link
Collaborator

Fixed

@yschimke
Copy link
Collaborator

I think we will remove doubleClick, just leaving the additional longClick. Mainly for unclear a11y reasons.

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

No branches or pull requests

6 participants