Open
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors the shortcuts settings reset UI to use the shared ghost icon-text button with proper hover behavior and adjusts the shortcut tile layout for better right-aligned, flexible keybinding display. Class diagram for updated reset button and shortcut tile layoutclassDiagram
class _ResetButton {
+VoidCallback? onReset
+build(BuildContext context) Widget
}
class AFGhostIconTextButton {
+AFGhostIconTextButton.primary(String text, VoidCallback onTap, Widget iconBuilder(BuildContext context, bool isHovering, bool disabled), EdgeInsets padding)
}
class FlowySvg {
+FlowySvg(FlowySvgs icon, Size size, Color? color)
}
class AFThemeExtension {
+Color textColor
+static AFThemeExtension of(BuildContext context)
}
class ShortcutSettingTile {
}
class _ShortcutSettingTileState {
+bool isEditing
+Widget _renderKeybindEditor()
+Widget _renderKeybindings(bool isHovering)
}
_ResetButton ..> AFGhostIconTextButton : uses_primary_constructor
_ResetButton ..> FlowySvg : builds_icon
_ResetButton ..> AFThemeExtension : reads_textColor
ShortcutSettingTile o-- _ShortcutSettingTileState : state
_ShortcutSettingTileState ..> _ResetButton : may_use_in_tile_layout
_ShortcutSettingTileState ..> Widget : uses_Flexible_and_Align_for_right_alignment
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Using
onReset ?? () {}means the button always appears enabled even when no callback is provided; consider passingonResetthrough directly so theAFGhostIconTextButtoncan naturally reflect the disabled state. - The
iconBuildercurrently ignores theisHoveringanddisabledarguments; if the design calls for a hover effect, consider adjusting the icon color or style based on these flags instead of always usingtextColor. - Switching from
ExpandedtoFlexiblewith anAlignwrapping_renderKeybindingschanges the layout behavior; double-check long or wrapping shortcut labels to ensure they don't overflow or get clipped when right-aligned with loose fit.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `onReset ?? () {}` means the button always appears enabled even when no callback is provided; consider passing `onReset` through directly so the `AFGhostIconTextButton` can naturally reflect the disabled state.
- The `iconBuilder` currently ignores the `isHovering` and `disabled` arguments; if the design calls for a hover effect, consider adjusting the icon color or style based on these flags instead of always using `textColor`.
- Switching from `Expanded` to `Flexible` with an `Align` wrapping `_renderKeybindings` changes the layout behavior; double-check long or wrapping shortcut labels to ensure they don't overflow or get clipped when right-aligned with loose fit.
## Individual Comments
### Comment 1
<location> `frontend/appflowy_flutter/lib/workspace/presentation/settings/pages/settings_shortcuts_view.dart:152-154` </location>
<code_context>
- ],
- ),
- ),
+ return AFGhostIconTextButton.primary(
+ text: LocaleKeys.settings_shortcutsPage_actions_resetDefault.tr(),
+ onTap: onReset ?? () {},
+ iconBuilder: (context, isHovering, disabled) => FlowySvg(
+ FlowySvgs.restore_s,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid using a no-op callback when `onReset` is null to preserve disabled semantics.
Passing `onReset ?? () {}` makes the button appear tappable even when no reset behavior exists, changing the previous behavior where `null` disabled taps. If `null` means “no reset available”, pass `onReset` directly so the disabled state and UX remain consistent with the original implementation.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Feature Preview
PR Checklist
Summary by Sourcery
Improve the shortcuts settings reset button and layout alignment in the shortcuts settings tile.
Enhancements: