Fix Player Panel vertical position (top out of view) #804
Conversation
WalkthroughThe player panel UI in the client graphics layer was refactored for improved layout and interaction. The container structure and styling were updated, new action buttons were added, label text was corrected, embargo button presentation was revised, and event handling was adjusted. All changes are internal to the render method. Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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/graphics/layers/PlayerPanel.ts (1)
408-411: Consider consistent width styling for embargo buttonsThe embargo buttons use
w-100while other action buttons usew-10. Ifw-100is intended to make these buttons wider (which makes sense for text labels), consider using a more standard Tailwind class likew-fullor a specific pixel width likew-[100px].- class="w-100 h-10 flex items-center justify-center + class="w-full h-10 flex items-center justify-center
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/client/graphics/layers/PlayerPanel.ts(1 hunks)
🔇 Additional comments (6)
src/client/graphics/layers/PlayerPanel.ts (6)
227-231: Great improvements to panel positioningThe change from absolute positioning to a fixed, full-screen flex container ensures the panel is always centered in the viewport, addressing the PR's main objective of fixing vertical positioning issues.
The addition of
pointer-events-noneon the container with the wheel event handler is an excellent approach to prevent interaction with the game map behind the panel while still capturing scroll events.
232-234: Well-implemented responsive constraintsLimiting the panel height to 90% of viewport (
max-h-[90vh]) withoverflow-y-autoensures the panel is fully visible regardless of screen resolution while still allowing access to all content if it exceeds the viewport.The
pointer-events-autoeffectively restores interaction only for the panel elements.
235-246: Improved modal structure with better close button positioningThe dedicated container for background styling and the absolute positioning of the close button slightly outside the panel (
-top-2 -right-2) improves accessibility and follows standard design patterns for modal interfaces.
248-320: Well-organized content sections with consistent stylingThe reorganized content with clear section comments and consistent flex layouts improves code readability and creates a more uniform design. The grid layout for resources is particularly effective.
383-403: Enhanced functionality with additional action buttonsThe addition of donate gold and emoji buttons expands the panel's functionality while maintaining consistent styling with the existing buttons.
405-425:✅ Verification successful
Verify the embargo button labels
The label texts for embargo actions may be counterintuitive. The button for initiating an embargo (lines 406-413) shows "stop_trade", while the button for stopping an embargo (lines 415-424) shows "start_trade". Consider verifying if these labels match the intended user actions.
🏁 Script executed:
#!/bin/bash # Check the translation keys for player panel embargo actions rg -A 2 "player_panel.stop_trade|player_panel.start_trade" --type jsonLength of output: 72
Embargo button labels are correctly aligned with their actions
The
canEmbargoflag represents whether trading is currently allowed.
- When
canEmbargois true, you’re allowed to initiate an embargo (stop trading), so showing"player_panel.stop_trade"(“Stop Trade”) is correct.- When
canEmbargois false, an embargo is in place and trading is blocked, so showing"player_panel.start_trade"(“Start Trade”) to lift the embargo is also correct.No changes needed.
|
Coderabbit is wrong here "new action buttons were added, label text was corrected, embargo button presentation was revised, and event handling was adjusted.". The only actual changes are on lines 228-237, the rest is shifts and from prettier. I'm not for following the nitpick comment to change Embargo button class. Don't fix what isn't broken as it doesn't have to do with the scope of this PR. But if you want me to, i can change it. |
Would it not be possible to disable the Coderabbitai @evanpelle? Or at least make it a bit less verbose and agressive? The copilot AI strikes a fine-enough balance between giving feedback and being annoying. |
github-project-automation
bot
moved this from Triage
to Final Review
in OpenFront Release Management
github-project-automation
bot
moved this from Final Review
to Complete
in OpenFront Release Management
## Description: Fixes the Player Info Panel top part being out of view, at least on some screen resolutions. Workaround was changing browser zoom from 100% to 80%, or to scroll to change the vertical position of the panel. This is now not needed anymore. Also disables vertical scrolling of the position of the panel itself. And prevents zooming the background game map. Position is still not perfect on all resolutions but the top or bottom won't be out of view anymore. Tested using DevTools. I don't have the option to deploy a branch (yet?), this may be needed so others can test on their screens just to be sure. Normally the current "top-1/2 -translate-y-1/2" should have done the trick and put the vertical position at 50% of the screen. The root cause of this not working on all screens may be a parent element or something else, but touching that could create new UI issues. So fixing it from within PlayerPanel.ts itself seemed the best choice here. !! The only actual changes are in lines 228-237. The rest are shifts in positions or from prettier. Before:   After (don't mind the empty player name box, the fix for that bug isn't in main yet #795):  ## Please complete the following: - [x] I have added screenshots for all UI updates - [x] I confirm I have thoroughly tested these changes and take full responsibility for any bugs introduced - [x] I understand that submitting code with bugs that could have been caught through manual testing blocks releases and new features for all contributors ## Please put your Discord username so you can be contacted if a bug or regression is found: tryout33
Description:
Fixes the Player Info Panel top part being out of view, at least on some screen resolutions. Workaround was changing browser zoom from 100% to 80%, or to scroll to change the vertical position of the panel. This is now not needed anymore.
Also disables vertical scrolling of the position of the panel itself. And prevents zooming the background game map.
Position is still not perfect on all resolutions but the top or bottom won't be out of view anymore. Tested using DevTools. I don't have the option to deploy a branch (yet?), this may be needed so others can test on their screens just to be sure.
Normally the current "top-1/2 -translate-y-1/2" should have done the trick and put the vertical position at 50% of the screen. The root cause of this not working on all screens may be a parent element or something else, but touching that could create new UI issues. So fixing it from within PlayerPanel.ts itself seemed the best choice here.
!! The only actual changes are in lines 228-237. The rest are shifts in positions or from prettier.
Before:
After (don't mind the empty player name box, the fix for that bug isn't in main yet #795):
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
tryout33