[UI] Feature: optimize sprite quick config#2900
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the sprite editing workflow by integrating quick configuration panels directly into the preview window and the global configuration map. This change aims to provide users with a more intuitive and efficient way to adjust sprite properties, reducing the need to navigate through multiple menus for common operations. The new panels offer direct controls for position, size, rotation, and layer order, significantly improving the overall usability for sprite management. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a demo page for the new sprite quick configuration feature. The implementation in demo.html is a good starting point, providing a clear demonstration of the intended UI interactions. The code is self-contained and uses modern JavaScript features effectively. I've provided a few suggestions to improve code maintainability and robustness by refactoring some parts to reduce duplication, remove magic numbers, and use constants for states. These changes will make the code easier to understand and extend in the future.
| let state = "default"; | ||
| let spriteDragOffsetX = 0; | ||
| let isDraggingSprite = false; | ||
| let dragStartClientX = 0; | ||
| let dragStartLeft = 0; | ||
| const spritePos = { | ||
| default: { x: 134, y: 34 }, | ||
| location: { x: 134, y: 34 }, | ||
| size: { x: 134, y: 34 }, | ||
| rotate: { x: 134, y: 34 }, | ||
| "horizontal-left": { x: 134, y: 46 }, | ||
| "horizontal-right": { x: 134, y: 46 }, | ||
| "not-rotate": { x: 134, y: 46 } | ||
| }; |
There was a problem hiding this comment.
Using magic strings for states like "default", "location", etc., is error-prone. A typo in a string won't be caught by any linter or compiler and can lead to bugs that are hard to track down. It's better to define these states as constants in an object. This will also make the code more self-documenting.
const STATES = {
DEFAULT: "default",
LOCATION: "location",
SIZE: "size",
ROTATE: "rotate",
HORIZONTAL_LEFT: "horizontal-left",
HORIZONTAL_RIGHT: "horizontal-right",
NOT_ROTATE: "not-rotate",
};
let state = STATES.DEFAULT;
let spriteDragOffsetX = 0;
let isDraggingSprite = false;
let dragStartClientX = 0;
let dragStartLeft = 0;
const spritePos = {
[STATES.DEFAULT]: { x: 134, y: 34 },
[STATES.LOCATION]: { x: 134, y: 34 },
[STATES.SIZE]: { x: 134, y: 34 },
[STATES.ROTATE]: { x: 134, y: 34 },
[STATES.HORIZONTAL_LEFT]: { x: 134, y: 46 },
[STATES.HORIZONTAL_RIGHT]: { x: 134, y: 46 },
[STATES.NOT_ROTATE]: { x: 134, y: 46 }
};| if (state === "horizontal-left") { | ||
| rangeLeft.style.left = "-10px"; | ||
| rangeRight.style.left = "189px"; | ||
| rangeLeft.classList.remove("inactive"); | ||
| rangeRight.classList.add("inactive"); | ||
| rangeLeft.classList.add("active-arrow"); | ||
| rangeRight.classList.remove("active-arrow"); | ||
| rangeLeft.classList.add("to-left"); | ||
| rangeLeft.classList.remove("to-right"); | ||
| rangeRight.classList.add("to-right"); | ||
| rangeRight.classList.remove("to-left"); | ||
| } | ||
| if (state === "horizontal-right") { | ||
| rangeLeft.style.left = "-10px"; | ||
| rangeRight.style.left = "189px"; | ||
| rangeLeft.classList.add("inactive"); | ||
| rangeRight.classList.remove("inactive"); | ||
| rangeLeft.classList.remove("active-arrow"); | ||
| rangeRight.classList.add("active-arrow"); | ||
| rangeLeft.classList.add("to-left"); | ||
| rangeLeft.classList.remove("to-right"); | ||
| rangeRight.classList.add("to-right"); | ||
| rangeRight.classList.remove("to-left"); | ||
| } |
There was a problem hiding this comment.
The if blocks for state === "horizontal-left" and state === "horizontal-right" contain a lot of duplicated code. This can be refactored to improve readability and maintainability. You can group the common logic and handle the differences with a boolean flag or ternary operators.
| if (state === "horizontal-left") { | |
| rangeLeft.style.left = "-10px"; | |
| rangeRight.style.left = "189px"; | |
| rangeLeft.classList.remove("inactive"); | |
| rangeRight.classList.add("inactive"); | |
| rangeLeft.classList.add("active-arrow"); | |
| rangeRight.classList.remove("active-arrow"); | |
| rangeLeft.classList.add("to-left"); | |
| rangeLeft.classList.remove("to-right"); | |
| rangeRight.classList.add("to-right"); | |
| rangeRight.classList.remove("to-left"); | |
| } | |
| if (state === "horizontal-right") { | |
| rangeLeft.style.left = "-10px"; | |
| rangeRight.style.left = "189px"; | |
| rangeLeft.classList.add("inactive"); | |
| rangeRight.classList.remove("inactive"); | |
| rangeLeft.classList.remove("active-arrow"); | |
| rangeRight.classList.add("active-arrow"); | |
| rangeLeft.classList.add("to-left"); | |
| rangeLeft.classList.remove("to-right"); | |
| rangeRight.classList.add("to-right"); | |
| rangeRight.classList.remove("to-left"); | |
| } | |
| if (state === "horizontal-left" || state === "horizontal-right") { | |
| rangeLeft.style.left = "-10px"; | |
| rangeRight.style.left = "189px"; | |
| const isLeft = state === "horizontal-left"; | |
| rangeLeft.classList.toggle("inactive", !isLeft); | |
| rangeRight.classList.toggle("inactive", isLeft); | |
| rangeLeft.classList.toggle("active-arrow", isLeft); | |
| rangeRight.classList.toggle("active-arrow", !isLeft); | |
| rangeLeft.classList.add("to-left"); | |
| rangeLeft.classList.remove("to-right"); | |
| rangeRight.classList.add("to-right"); | |
| rangeRight.classList.remove("to-left"); | |
| } |
| if (action === "to-default") state = "default"; | ||
| if (action === "to-location") state = "location"; | ||
| if (action === "to-size") state = "size"; | ||
| if (action === "to-rotate") state = "rotate"; | ||
| if (action === "to-horizontal-left") state = "horizontal-left"; | ||
| if (action === "to-horizontal-right") state = "horizontal-right"; | ||
| if (action === "to-not-rotate") state = "not-rotate"; | ||
| if (action === "toggle-layer" && state === "default") { | ||
| expandList.classList.toggle("show"); | ||
| } |
There was a problem hiding this comment.
The series of if statements to handle different actions can be refactored for better readability and maintainability. Using a switch statement is a cleaner approach, especially as more actions might be added in the future.
| if (action === "to-default") state = "default"; | |
| if (action === "to-location") state = "location"; | |
| if (action === "to-size") state = "size"; | |
| if (action === "to-rotate") state = "rotate"; | |
| if (action === "to-horizontal-left") state = "horizontal-left"; | |
| if (action === "to-horizontal-right") state = "horizontal-right"; | |
| if (action === "to-not-rotate") state = "not-rotate"; | |
| if (action === "toggle-layer" && state === "default") { | |
| expandList.classList.toggle("show"); | |
| } | |
| switch (action) { | |
| case "to-default": | |
| case "to-location": | |
| case "to-size": | |
| case "to-rotate": | |
| case "to-horizontal-left": | |
| case "to-horizontal-right": | |
| case "to-not-rotate": | |
| state = action.substring(3); | |
| break; | |
| case "toggle-layer": | |
| if (state === "default") { | |
| expandList.classList.toggle("show"); | |
| } | |
| break; | |
| } |
| const dx = e.clientX - dragStartClientX; | ||
| const nextLeft = dragStartLeft + dx; | ||
| const minX = 0; | ||
| const maxX = 468 - 199; |
There was a problem hiding this comment.
The pointermove event handler uses hardcoded values for calculating maxX. This makes the code less maintainable and resilient to changes in CSS. It's better to dynamically get the dimensions from the DOM elements.
| const maxX = 468 - 199; | |
| const maxX = document.getElementById("window").clientWidth - spriteBox.clientWidth; |
2 participants
update #2878
Background
Supports users to perform quick sprite operations in the preview window and the global configuration map.
Modifications
Added a quick operation panel to the preview window.
Added a quick operation panel to the global configuration map.
Scope of Impact
Sprite editor page
Global configuration page
Does it affect the design system?