Add confirmation step to destroy building #2433
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes in this pull request modify the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
You are out of MentatBot reviews. Your usage will refresh December 16 at 08:00 AM. |
![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
🧹 Outside diff range and nitpick comments (2)
client/src/ui/modules/entity-details/BuildingEntityDetails.tsx (2)
Line range hint
96-115: Add error handling and loading stateThe destroy building operation lacks error handling and loading state management. This could lead to a poor user experience if the operation fails.
Consider implementing these improvements:
const handleDestroyBuilding = useCallback(() => { if (!showDestroyConfirm) { setShowDestroyConfirm(true); return; } + setIsLoading(true); const tileManager = new TileManager(dojo.setup, { col: selectedBuildingHex.outerCol, row: selectedBuildingHex.outerRow, }); - tileManager.destroyBuilding(selectedBuildingHex.innerCol, selectedBuildingHex.innerRow); + tileManager + .destroyBuilding(selectedBuildingHex.innerCol, selectedBuildingHex.innerRow) + .then(() => { if ( buildingState.buildingType === BuildingType.Resource && (ResourceIdToMiningType[buildingState.resource!] === ResourceMiningTypes.Forge || ResourceIdToMiningType[buildingState.resource!] === ResourceMiningTypes.Mine) ) { playDestroyStone(); } else { playDestroyWooden(); } setShowDestroyConfirm(false); setLeftNavigationView(LeftView.None); + }) + .catch((error) => { + console.error('Failed to destroy building:', error); + // Consider showing an error toast/notification here + }) + .finally(() => { + setIsLoading(false); + setShowDestroyConfirm(false); + });
180-180: Enhance button accessibility and visual distinctionWhile the button text change works, consider improving accessibility and visual distinction for the confirmation state.
Consider these improvements:
-{showDestroyConfirm ? "Confirm Destroy" : "Destroy"} +{showDestroyConfirm ? ( + <span aria-live="polite" className="text-red-600 font-bold"> + Confirm Destroy + </span> +) : ( + "Destroy" +)}This change will:
- Make the state change announcement clearer for screen readers
- Provide visual emphasis for the confirmation state
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
client/src/ui/modules/entity-details/BuildingEntityDetails.tsx(5 hunks)
🔇 Additional comments (1)
client/src/ui/modules/entity-details/BuildingEntityDetails.tsx (1)
34-34: LGTM: Clear and well-structured state management
The state variable is appropriately named and follows React best practices.
|
Failed to generate code suggestions for PR |
Closes #2432
Summary by CodeRabbit