Skip to content

Conversation

@rohan-ramani
Copy link

The minimap is now draggable across the entire user interface. This way it doesn't obstruct the view on the bottom left and the player can decide where they want to put it. The score UI seemed outdated so I changed the CSS a little better.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

@windsurf-bot windsurf-bot bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other comments: 0 Blocking, 2 Optional, 0 Nit, 0 FYI
  • Optional: static/js/ui.js (28-28) Optional: The mousemove and mouseup event listeners are added to the document but never removed. This could lead to memory leaks or unexpected behavior if the minimap is initialized multiple times or removed from the DOM. Consider adding a cleanup function or using the `{ once: true }` option for the mouseup listener.
  • Optional: templates/game.html (287-289) Optional: The toggleDarkMode function now checks for any truthy value instead of specifically checking for 'dark'. This could cause issues if data-theme gets set to any other value. Consider using the specific check:
        function toggleDarkMode() {
            document.documentElement.setAttribute('data-theme', document.documentElement.getAttribute('data-theme') === 'dark' ? '' : 'dark');
        }
    

Comment on lines +21 to +26
minimap.addEventListener('mousedown', (e) => {
isDragging = true;
initialX = e.clientX - currentX;
initialY = e.clientY - currentY;
minimap.style.cursor = 'grabbing';
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The mousedown event handler uses currentX and currentY before they have proper values. When a user first clicks on the minimap, these variables might be undefined, causing the minimap to jump to an unexpected position.

Suggested change
minimap.addEventListener('mousedown', (e) => {
isDragging = true;
initialX = e.clientX - currentX;
initialY = e.clientY - currentY;
minimap.style.cursor = 'grabbing';
});
minimap.addEventListener('mousedown', (e) => {
isDragging = true;
// Get current position from style or computed style if not set yet
if (currentX === undefined || currentY === undefined) {
const minimapRect = minimap.getBoundingClientRect();
currentX = minimapRect.left;
currentY = minimapRect.top;
}
initialX = e.clientX - currentX;
initialY = e.clientY - currentY;
minimap.style.cursor = 'grabbing';
});

Comment on lines +263 to +264
<span>Dark Mode</span>
<input type="checkbox" id="dark-mode-toggle" onKeyPress="handleDarkModeKeyPress(event)">
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dark mode toggle checkbox is missing the onclick handler. You've added keyboard support with onKeyPress but removed the click functionality. Consider adding the onclick handler back:

Suggested change
<span>Dark Mode</span>
<input type="checkbox" id="dark-mode-toggle" onKeyPress="handleDarkModeKeyPress(event)">
<span>Dark Mode</span>
<input type="checkbox" id="dark-mode-toggle" onclick="toggleDarkMode()" onKeyPress="handleDarkModeKeyPress(event)">

localStorage.setItem('darkMode', isDarkMode);
}

function initMinimapDrag() {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The minimap dragging implementation only handles mouse events (mousedown, mousemove, mouseup) but doesn't support touch events. This means the feature won't work on mobile devices. Consider adding touch event support (touchstart, touchmove, touchend) for better cross-platform compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants