fix: improve settings button hover consistency and shortcut layout#8484
Open
brianyoon0 wants to merge 1 commit intoAppFlowy-IO:mainfrom
Open
fix: improve settings button hover consistency and shortcut layout#8484brianyoon0 wants to merge 1 commit intoAppFlowy-IO:mainfrom
brianyoon0 wants to merge 1 commit intoAppFlowy-IO:mainfrom
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAligns the settings reset button hover behavior and styles with other settings buttons, and adjusts the shortcut settings tile layout to use a space-between alignment instead of two fixed Expanded columns. Flow diagram for shortcut tile layout change to space betweenflowchart LR
A_SettingsShortcutsPage["SettingsShortcutsPage"] --> B_ShortcutSettingTile["ShortcutSettingTile (StatefulWidget)"]
B_ShortcutSettingTile --> C_RowBefore["Row before change\nmainAxisAlignment: start\nchildren: [Expanded(label), Expanded(editor_or_bindings)]"]
B_ShortcutSettingTile --> D_RowAfter["Row after change\nmainAxisAlignment: spaceBetween\nchildren: [Label_section, Keybind_section]"]
C_RowBefore --> E_BehaviorBefore["Label and keybind each take 50% width"]
D_RowAfter --> F_BehaviorAfter["Label uses intrinsic width, keybinds aligned to right"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
Rowin_ShortcutSettingTileStateappears to be misconfigured:MainAxisAlignment: MainAxisAlignment.spaceBetweenand an extrachildren: [are placed inside thechildrenlist instead of as named parameters onRow, which will not compile; this should be refactored somainAxisAlignmentis passed toRowand there is only a singlechildren: [...]. - There is a typo in the ternary for rendering the keybind editor (
isEditiinginstead ofisEditing), which will break the condition; update the identifier to match the existing state field.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Row` in `_ShortcutSettingTileState` appears to be misconfigured: `MainAxisAlignment: MainAxisAlignment.spaceBetween` and an extra `children: [` are placed inside the `children` list instead of as named parameters on `Row`, which will not compile; this should be refactored so `mainAxisAlignment` is passed to `Row` and there is only a single `children: [...]`.
- There is a typo in the ternary for rendering the keybind editor (`isEditiing` instead of `isEditing`), which will break the condition; update the identifier to match the existing state field.
## Individual Comments
### Comment 1
<location> `frontend/appflowy_flutter/lib/workspace/presentation/settings/pages/settings_shortcuts_view.dart:382-384` </location>
<code_context>
- Expanded(
- child: isEditing
- ? _renderKeybindEditor()
+ isEditiing
+ ? Expanded(child: _renderKeybindEditor())
: _renderKeybindings(isHovering),
</code_context>
<issue_to_address>
**issue (typo):** `isEditiing` appears to be a typo and will likely not compile if `isEditing` is the actual state field.
The rest of this class uses `isEditing` as the state flag, so introducing `isEditiing` here creates a new, undefined identifier and will fail to compile. This should stay as `isEditing` for consistency with the existing implementation.
```suggestion
isEditing
? Expanded(child: _renderKeybindEditor())
: _renderKeybindings(isHovering),
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
...tend/appflowy_flutter/lib/workspace/presentation/settings/pages/settings_shortcuts_view.dart
Outdated
Show resolved
Hide resolved
…ppFlowy-IO#8222) - Fixed reset button hover effect to match other settings buttons - Changed shortcut tile layout from 50/50 to space-between alignment - Improved visual consistency across settings UI Fixes AppFlowy-IO#8222
f76c0ef to
208cca6
Compare
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
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.
…8222)
Fixes #8222
Feature Preview
PR Checklist
Summary by Sourcery
Improve the settings shortcuts UI for better hover feedback and shortcut layout.
Enhancements: