Keep DropdownMenu open after selection#604
Conversation
kitakkun
left a comment
kitakkun
left a comment
There was a problem hiding this comment.
Looks great! Thank you. 👍🏻
|
This PR has already been approved, but we’ve noticed some UX concerns and are currently discussing the direction within the team. To explain briefly, with the current implementation, after selecting an item from the dropdown, the user needs to manually tap outside the menu each time to close it. There’s a concern that this extra step may not provide a good user experience. Also, in typical use cases, users are unlikely to select multiple session categories at once, so the existing behavior might actually be more preferable from a UX perspective. Sorry for the inconvenience, and thank you for your patience. We’ll reach out again once we’ve settled on a direction. 🙏🏻 |
|
@riko111
Would you be able to update your implementation to match these specifications? Thanks in advance! |
|
I'll do my best. |
|
This PR updates the DropdownMenu behavior to support the requested multi-selection mode specifically for category items:
These changes align the implementation with the discussed specifications and improve usability when selecting multiple categories. 20250909-103047.mp4By the way, do we still need the session type dropdown list? It might be unnecessary, especially since it doesn't exist in the iPhone version. |
| fun SearchFilterRow( | ||
| filters: SearchScreenUiState.Filters, | ||
| onFilterToggle: (SearchScreenEvent.Filter) -> Unit, | ||
| onFilterLongPress: ((SearchScreenEvent.Filter) -> Unit)? = null, |
There was a problem hiding this comment.
I noticed onFilterLongPress is always null 👀. Maybe we could consider removing this argument?
There was a problem hiding this comment.
Thanks for pointing that out, I hadn’t noticed it.
I’ll remove the parameter along with the comment below.
| modifier = if (onItemLongPress != null) { | ||
| Modifier.combinedClickable( | ||
| onLongClick = { | ||
| if (selectedItems.isNotEmpty()) { | ||
| onItemLongPress(selectedItems.first()) | ||
| } | ||
| }, | ||
| ) { | ||
| scope.launch { | ||
| withFrameNanos { | ||
| keyboardController?.hide() | ||
| } | ||
| }.invokeOnCompletion { expanded = true } | ||
| } | ||
| } else { | ||
| Modifier | ||
| }, |
There was a problem hiding this comment.
Since onItemLongPress is always null, maybe we could consider removing these lines of code.
| } | ||
| }, | ||
| onLongClick = { | ||
| if (!isMultiSelectMode) isMultiSelectMode = true |
There was a problem hiding this comment.
It seems like this if check might not be necessary. Maybe we could just set isMultiSelectMode = true directly?
| .padding(horizontal = 12.dp, vertical = 8.dp) | ||
| .combinedClickable( |
There was a problem hiding this comment.
I think it might be better to invoke combinedClickable before applying the padding modifier. That way, the ripple effect would behave more properly.
There was a problem hiding this comment.
Thanks, that’s really helpful and I learned something!
| if (item in selectedItems) { | ||
| Icon(Icons.Default.Check, contentDescription = null, modifier = Modifier.padding(end = 12.dp)) | ||
| } |
There was a problem hiding this comment.
Hiding the icon entirely when unselected might cause the view width to shift compared to the existing behavior, which could feel janky. Maybe controlling the alpha instead would be smoother.
Screen_recording_20250909_150111.mp4
There was a problem hiding this comment.
Along with this, I also locked the chip width while the menu is open, so the dropdown list position no longer shifts horizontally.
There’s a corresponding issue opened for this: #500. |
…dth while menu is open - Fade out unselected check icons instead of removing them to avoid width shifts - Lock the chip width while the dropdown menu is expanded so the anchor position stays stable
…dth while menu is open - Fade out unselected check icons instead of removing them to avoid width shifts - Lock the chip width while the dropdown menu is expanded so the anchor position stays stable
|
Addressed the review feedback and updated the implementation accordingly. screen-20250909-172113.mp4 |
| if (selectedItems.isNotEmpty()) { | ||
| Icon( | ||
| imageVector = Icons.Default.Check, | ||
| contentDescription = null, | ||
| ) | ||
| } |
There was a problem hiding this comment.
I appreciate your effort with this change 🙏🏻, but for this checkmark, we don’t need to control its visibility via alpha. It’s fine to keep the original display logic as it was, since this introduces an unintended visual change.
| modifier = Modifier | ||
| .then( | ||
| if (lockedChipWidthDp != null) { | ||
| Modifier.width(lockedChipWidthDp!!) | ||
| } else { | ||
| Modifier | ||
| }, | ||
| ) | ||
| .onGloballyPositioned { cords -> | ||
| currentChipWidthPx = cords.size.width | ||
| if (expanded && lockedChipWidthDp == null) { | ||
| lockedChipWidthDp = with(density) { currentChipWidthPx.toDp() } | ||
| } | ||
| }, |
There was a problem hiding this comment.
While I understand the issue of the dropdown menu moving during selection, this implementation is somewhat tricky and unnecessarily increases complexity, so I’d like to avoid that.
It seems that placing the DropdownMenu directly at the same hierarchy level as the FilterChip causes the FilterChip to become the anchor for the menu, which is why this problem occurs.
Wrapping the DropdownMenu in a Box could be a good solution. It might seem a bit unusual, but this way the dropdown’s anchor becomes a stable, empty Box, which keeps its position consistent.
Box {
FilterChip()
// stable anchor point for DropdownMenu
Box {
DropdownMenu()
}
}| DropdownMenuItem( | ||
| modifier = Modifier.testTag(DropdownFilterChipTestTagPrefix.plus(item)), | ||
| leadingIcon = { | ||
| if (selectedItems.contains(item)) { | ||
| Icon( | ||
| imageVector = Icons.Default.Check, | ||
| contentDescription = null, | ||
| ) | ||
| } | ||
| }, | ||
| text = { Text(itemLabel(item)) }, | ||
| onClick = { | ||
| onItemSelected(item) | ||
| expanded = false | ||
| }, | ||
| ) | ||
| Row( | ||
| modifier = Modifier | ||
| .testTag(DropdownFilterChipTestTagPrefix.plus(item)) | ||
| .fillMaxWidth() | ||
| .heightIn(min = 48.dp) | ||
| .combinedClickable( | ||
| onClick = { | ||
| onItemSelected(item) | ||
| if (!isMultiSelectMode) { | ||
| expanded = false | ||
| } | ||
| }, | ||
| onLongClick = { | ||
| isMultiSelectMode = true | ||
| haptics.performHapticFeedback(HapticFeedbackType.LongPress) | ||
| onItemSelected(item) | ||
| }, | ||
| ) | ||
| .padding(horizontal = 12.dp, vertical = 8.dp), | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| Icon( | ||
| Icons.Default.Check, | ||
| contentDescription = null, | ||
| modifier = Modifier | ||
| .padding(end = 12.dp) | ||
| .alpha(if (selectedItems.contains(item)) 1f else 0f), | ||
| ) | ||
| Text(itemLabel(item), style = MaterialTheme.typography.bodyLarge) | ||
| } |
There was a problem hiding this comment.
This is a slightly unconventional approach, but I came up with a way to make DropdownMenuItem respond to long presses. I’m not claiming this method is objectively better, so whether to adopt it is entirely up to you.
By implementing it like this, you can enable long-clicks on DropdownMenuItem without creating a custom menu item. The idea is to overlay a box of the same size on top of the menu item and handle click events there:
Box(
modifier = Modifier.testTag(DropdownFilterChipTestTagPrefix.plus(item))
) {
DropdownMenuItem(
leadingIcon = {
if (selectedItems.contains(item)) {
Icon(
imageVector = Icons.Default.Check,
contentDescription = null,
)
}
},
text = { Text(itemLabel(item)) },
onClick = { /* actual click events are handled in the overlayed Box */ },
)
Box(
Modifier
.matchParentSize()
.combinedClickable(
onClick = {
onItemSelected(item)
if (!isMultiSelectMode) {
expanded = false
}
},
onLongClick = {
isMultiSelectMode = true
haptics.performHapticFeedback(HapticFeedbackType.LongPress)
onItemSelected(item)
},
)
)
}In short, the top Box intercepts both normal and long clicks, so you don’t need to customize DropdownMenuItem itself.
What do you think?
There was a problem hiding this comment.
Actually, I first tried writing it the way you suggested, but I couldn’t get it to work, so I ended up using the current approach.
There was a problem hiding this comment.
But with the other feedback helping me tidy up the code, I feel more confident it will work this time.
There was a problem hiding this comment.
I’m not sure why, but it seems that adding the following Modifier to the DropdownMenuItem not to the outer Box allows the existing tests to pass without issues:
modifier = Modifier.testTag(DropdownFilterChipTestTagPrefix.plus(item))| items: List<T>, | ||
| itemLabel: (T) -> String, | ||
| onItemSelected: (T) -> Unit, | ||
| onMultiSelectFinished: (List<T>) -> Unit = {}, |
There was a problem hiding this comment.
Specifying default values for lambda parameters has some downsides: it can make it easy to forget to provide a value, and at a glance it’s not always clear whether a value is actually being passed. Therefore, unless there’s a specific reason, I’d prefer to remove default empty lambdas.
|
I’ve left a few comments—when you have a chance, please take a look 🙏🏻 I realize they’re a bit nitpicky, but they touch on aspects that are actually important for UX and code maintainability. I appreciate your understanding! |
…invoke only when provided
…ble and invoke only when provided" This reverts commit 3c3894b.
|
I really appreciate you reviewing multiple times during the busy day before the first day. After reorganizing the code, the approach that didn’t work at first ended up working. |
|
@riko111 |
|
Just a quick comment. If it’s only the changes for Issue #500, it could be merged independently, so feel free to consider doing that! |
|
Sorry, I think I mistakenly sent a review request. I haven’t had a chance to look into it yet, so please withdraw it. |
2 participants
Issue
Overview (Required)
Links
Movie (Optional)
2025-09-08.1.42.31.mov
20250908_164950.mp4