-
-
Notifications
You must be signed in to change notification settings - Fork 935
feat: Show tooltip for unobvious elements #2637
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
base: dev
Are you sure you want to change the base?
Conversation
I can't believe this happened
Agree, willco on Sunday weekends. |
# Conflicts: # app/src/main/java/app/revanced/manager/ui/component/ExceptionViewerDialog.kt
Done™️! |
Co-authored-by: Copilot <[email protected]>
app/src/main/java/app/revanced/manager/ui/component/tooltip/TooltipIconButton.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/app/revanced/manager/ui/component/AppScaffold.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: Ax333l <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds tooltip functionality to improve accessibility for unobvious UI elements by wrapping icon buttons, floating action buttons, and small floating action buttons with tooltip components. The changes introduce new TooltipWrap
composable components and their specialized variants for different button types.
- Introduces new
TooltipWrap
component and specialized tooltip variants (TooltipIconButton
,TooltipFloatingActionButton
,TooltipSmallFloatingActionButton
) - Replaces regular buttons with tooltip-wrapped equivalents across multiple screens to provide contextual help text
- Adds haptic feedback support to tooltip components for enhanced user experience
Reviewed Changes
Copilot reviewed 23 out of 24 changed files in this pull request and generated 10 comments.
Show a summary per file
File | Description |
---|---|
TooltipWrap.kt | New base tooltip wrapper component with string and resource ID variants |
TooltipIconButton.kt | IconButton wrapper with tooltip functionality |
TooltipFloatingActionButton.kt | FloatingActionButton wrapper with tooltip functionality |
TooltipSmallFloatingActionButton.kt | SmallFloatingActionButton wrapper with tooltip functionality |
HapticSmallFloatingActionButton.kt | New haptic feedback component for small FABs |
Various screen files | Updated to use tooltip-wrapped buttons instead of regular buttons |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
* Wraps a composable with a tooltip. | ||
* | ||
* @param modifier the [Modifier] to applied to Tooltip. | ||
* @param tooltip [String] text to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [String] text
.
Copilot uses AI. Check for mistakes.
* Wraps a composable with a tooltip. | ||
* | ||
* @param modifier the [Modifier] to applied to tooltip. | ||
* @param tooltip [Int] or `id` string resource to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [Int] or id
.
* @param tooltip [Int] or `id` string resource to show in a tooltip. | |
* @param tooltip the string resource ID to show in a tooltip. |
Copilot uses AI. Check for mistakes.
/** | ||
* [IconButton] with tooltip-specific params. | ||
* | ||
* @param tooltip [String] text to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [String] text
.
* @param tooltip [String] text to show in a tooltip. | |
* @param tooltip Text to show in a tooltip. |
Copilot uses AI. Check for mistakes.
/** | ||
* [IconButton] with tooltip-specific params. | ||
* | ||
* @param tooltip [Int] or `id` string resource to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [Int] or id
.
* @param tooltip [Int] or `id` string resource to show in a tooltip. | |
* @param tooltip tooltip string resource to show in a tooltip. |
Copilot uses AI. Check for mistakes.
/** | ||
* [HapticFloatingActionButton] with tooltip-specific params. | ||
* | ||
* @param tooltip [String] text to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [String] text
.
* @param tooltip [String] text to show in a tooltip. | |
* @param tooltip Text to show in a tooltip. |
Copilot uses AI. Check for mistakes.
/** | ||
* [HapticFloatingActionButton] with tooltip-specific params. | ||
* | ||
* @param tooltip [Int] or `id` string resource to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [Int] or id
.
* @param tooltip [Int] or `id` string resource to show in a tooltip. | |
* @param tooltip String resource ID to show in a tooltip. |
Copilot uses AI. Check for mistakes.
/** | ||
* [HapticSmallFloatingActionButton] with tooltip-specific params. | ||
* | ||
* @param tooltip [String] text to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [String] text
.
* @param tooltip [String] text to show in a tooltip. | |
* @param tooltip Text to show in a tooltip. |
Copilot uses AI. Check for mistakes.
/** | ||
* [HapticSmallFloatingActionButton] with tooltip-specific params. | ||
* | ||
* @param tooltip [Int] or `id` string resource to show in a tooltip. |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter description uses incorrect formatting. It should be tooltip
instead of [Int] or id
.
* @param tooltip [Int] or `id` string resource to show in a tooltip. | |
* @param tooltip Resource id of the string to show in a tooltip. |
Copilot uses AI. Check for mistakes.
modifier = Modifier.draggableHandle(interactionSource = interactionSource), | ||
onClick = {}, | ||
tooltip = stringResource(R.string.delete), | ||
onClick = { } |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty onClick handler should be replaced with a meaningful implementation or removed if not needed.
onClick = { } | |
onClick = { | |
if (!deleteMode) { | |
deletionTargets.add(item.key) | |
deleteMode = true | |
} else { | |
if (item.key in deletionTargets) { | |
deletionTargets.remove(item.key) | |
deleteMode = deletionTargets.isNotEmpty() | |
} else { | |
deletionTargets.add(item.key) | |
} | |
} | |
} |
Copilot uses AI. Check for mistakes.
TooltipIconButton( | ||
onClick = onSettingsClick, | ||
tooltip = stringResource(R.string.settings), | ||
) { | ||
BadgedBox( | ||
badge = { | ||
Badge(modifier = Modifier.size(6.dp)) | ||
} | ||
) { |
Copilot
AI
Aug 29, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The settings icon shows a badge without any conditional logic. This will always display a badge even when there are no settings notifications, which is likely unintended behavior.
TooltipIconButton( | |
onClick = onSettingsClick, | |
tooltip = stringResource(R.string.settings), | |
) { | |
BadgedBox( | |
badge = { | |
Badge(modifier = Modifier.size(6.dp)) | |
} | |
) { | |
// TODO: Replace with actual logic to determine if a settings notification exists | |
val hasSettingsNotification = false | |
TooltipIconButton( | |
onClick = onSettingsClick, | |
tooltip = stringResource(R.string.settings), | |
) { | |
if (hasSettingsNotification) { | |
BadgedBox( | |
badge = { | |
Badge(modifier = Modifier.size(6.dp)) | |
} | |
) { | |
Icon(Icons.Outlined.Settings, stringResource(R.string.settings)) | |
} | |
} else { |
Copilot uses AI. Check for mistakes.
visible = !visible | ||
}) { | ||
TooltipIconButton( | ||
modifier = Modifier, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unnecessary to add empty Modifiers
, since that is already the default. There are several of these across the codebase that need to be removed as well.
}, | ||
supportingContent = { Text(stringResource(if (patchBundle != null) R.string.file_field_set else R.string.file_field_not_set)) }, | ||
trailingContent = { | ||
// TODO: Determine if this button should be [TooltipWrap]'ped |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this should be adressed.
import app.revanced.manager.util.withHapticFeedback | ||
|
||
@Composable | ||
fun HapticSmallFloatingActionButton ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fun HapticSmallFloatingActionButton ( | |
fun HapticSmallFloatingActionButton( |
TooltipIconButton( | ||
modifier = Modifier.draggableHandle(interactionSource = interactionSource), | ||
onClick = {}, | ||
tooltip = stringResource(R.string.delete), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using R.string.delete
doesn't seem right here. This is just a drag handle.
import app.revanced.manager.ui.component.haptics.HapticFloatingActionButton | ||
|
||
/** | ||
* [HapticFloatingActionButton] with tooltip-specific params. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe [HapticComponent]
with a tooltip or something.
fun TooltipWrap( | ||
modifier: Modifier = Modifier, | ||
@StringRes tooltip: Int, | ||
positionProvider: PopupPositionProvider = TooltipDefaults.rememberPlainTooltipPositionProvider(), | ||
haptic: Boolean = true, | ||
hapticFeedbackType: HapticFeedbackType = HapticFeedbackType.LongPress, | ||
content: @Composable () -> Unit | ||
) { | ||
val tooltipState = rememberTooltipState() | ||
val localHaptic = LocalHapticFeedback.current | ||
|
||
LaunchedEffect(tooltipState.isVisible) { | ||
if (tooltipState.isVisible && haptic) { | ||
localHaptic.performHapticFeedback(hapticFeedbackType) | ||
} | ||
} | ||
|
||
TooltipBox( | ||
modifier = modifier, | ||
positionProvider = positionProvider, | ||
tooltip = { PlainTooltip { Text(stringResource(tooltip)) } }, | ||
state = tooltipState, | ||
content = content, | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The string resource variants seem unused, so you can get rid of them.
supportingContent = patch.description?.let { { Text(it) } }, | ||
trailingContent = { | ||
if (patch.options?.isNotEmpty() == true) { | ||
// TODO: Determine if this button should be [TooltipWrap] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
Add TooltipWrap to well... wrap a composable component using TooltipBox.
Extended FAB aren't wrapped because they have text shown in the button.
Icon Button, Small FAB and regular FAB are wrapped because they do not have text shown.
partially fix - #2555