Replace HelpModal keybind explanations with settings link #2886
Conversation
|
|
WalkthroughRefactors HelpModal to remove internal keybind parsing and rendering, delegating keybinds access to the user settings modal. Adds three English localization strings for a new keybinds subsection and a method on UserSettingModal to open the keybinds tab. Changes
Sequence DiagramsequenceDiagram
actor User
participant HelpModal
participant UserSettingModal
participant UI
User->>HelpModal: Click "Keybinds" button
HelpModal->>HelpModal: openSettingsModal()
alt UserSettingModal available
HelpModal->>UserSettingModal: openKeybindsTab()
UserSettingModal->>UserSettingModal: set activeTab = "keybinds"
UserSettingModal->>UI: render modal with keybinds tab
else Fallback
HelpModal->>UI: navigate to settings page
end
UI-->>User: show settings/keybinds UI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-06-09T02:20:43.637ZApplied to files:
📚 Learning: 2026-01-12T21:37:01.156ZApplied to files:
🧬 Code graph analysis (1)src/client/HelpModal.ts (2)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/client/HelpModal.ts (1)
11-25: Consider closing HelpModal before opening settings.The fallback chain is well-structured. However, when opening the settings modal, the HelpModal stays open. This could cause modal stacking issues where both modals are visible at the same time.
Suggested fix
private openSettingsModal = () => { + this.close(); const settingsModal = document.querySelector("user-setting") as { open?: () => void; openKeybindsTab?: () => void; } | null; if (settingsModal?.openKeybindsTab) { settingsModal.openKeybindsTab(); return; } if (settingsModal?.open) { settingsModal.open(); return; } window.showPage?.("page-settings"); };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
resources/lang/en.jsonsrc/client/HelpModal.tssrc/client/UserSettingModal.ts
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
📚 Learning: 2026-01-12T21:37:01.156Z
Learnt from: FloPinguin
Repo: openfrontio/OpenFrontIO PR: 2874
File: src/server/MapLandTiles.ts:7-11
Timestamp: 2026-01-12T21:37:01.156Z
Learning: In this repository's OpenFrontIO deployment, inter-service HTTP calls to the master should target http://localhost:3000 (master at port 3000) as the canonical address. Apply this as the standard for all server-side TypeScript code that communicates with the master. Avoid hardcoding non-master URLs; centralize the master address (e.g., via config or env) when possible, and ensure internal service communication uses localhost:3000 in this architecture.
Applied to files:
src/client/UserSettingModal.tssrc/client/HelpModal.ts
📚 Learning: 2025-06-02T14:27:37.609Z
Learnt from: andrewNiziolek
Repo: openfrontio/OpenFrontIO PR: 1007
File: resources/lang/de.json:115-115
Timestamp: 2025-06-02T14:27:37.609Z
Learning: For OpenFrontIO project: When localization keys are renamed in language JSON files, the maintainers separate technical changes from translation content updates. They wait for community translators to update the actual translation values rather than attempting to translate in the same PR. This allows technical changes to proceed while ensuring accurate translations from native speakers.
Applied to files:
resources/lang/en.json
📚 Learning: 2025-06-09T02:20:43.637Z
Learnt from: VariableVince
Repo: openfrontio/OpenFrontIO PR: 1110
File: src/client/Main.ts:293-295
Timestamp: 2025-06-09T02:20:43.637Z
Learning: In src/client/Main.ts, during game start in the handleJoinLobby callback, UI elements are hidden using direct DOM manipulation with classList.add("hidden") for consistency. This includes modals, buttons, and error divs. The codebase follows this pattern rather than using component APIs for hiding elements during game transitions.
Applied to files:
src/client/HelpModal.ts
🧬 Code graph analysis (1)
src/client/HelpModal.ts (2)
src/client/LangSelector.ts (1)
translateText(276-300)src/client/Utils.ts (1)
translateText(165-224)
🔇 Additional comments (4)
resources/lang/en.json (1)
62-64: New translation keys look good.The three new keys (
keybinds_title,keybinds_desc,keybinds_button) follow the existing naming pattern and provide clear, user-friendly text. The British spelling "customisable" is consistent.Note: The old
hotkeyskey at line 61 may now be unused. Consider removing it in a follow-up cleanup if confirmed dead.src/client/HelpModal.ts (2)
51-98: Clean UI update for the keybinds section.The simplified keybinds section replaces the old table with a brief description and action button. The styling is consistent with other sections, and translation keys are used correctly. Good use of
type="button"for accessibility.
1-7: Import cleanup is correct.Removed the
statedecorator import since keybinds state management was moved out. The remaining imports are all used.src/client/UserSettingModal.ts (1)
952-956: Simple and effective method for direct keybinds tab access.The
openKeybindsTab()method cleanly sets the active tab before opening, ensuring users land directly on the keybinds settings. This follows good composition patterns - the HelpModal delegates navigation to this modal rather than duplicating keybind rendering.
|
@Aotumuri @ryanbarlow97 What about the explanatory pictures of a right or left mouse click, or mouse wheel scroll or click, or mouse click combined with a key? Those were there to actually instruct, to help. I'd say we need those pictures in Keybinds Settings if not in Instructions anymore |
|
|
|
Yes agree, I think if needed we can move some static ones into there for now and then in another PR make them customisable? |
5 participants
Description:
This PR removes the remaining keybind explanations from the HelpModal and replaces them with a short description and a button that directs users to the keybind settings.
Following the previous PR that made fixed keybinds configurable, this change completes the HelpModal cleanup by no longer duplicating keybind information in help text.
Instead of listing individual keybinds, the HelpModal now guides users to the Keybinds tab in Settings, where the full and up-to-date list of bindings can be viewed and customized.
This helps:
2026-01-13.21.46.37.mov
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
aotumuri