refactor: Convert several components to functional components#196
refactor: Convert several components to functional components#196ShrimpCryptid wants to merge 5 commits into
Conversation
ShrimpCryptid
added
the
frontend
There was a problem hiding this comment.
Pull request overview
Refactors parts of the frontend UI from legacy class-based JavaScript components into TypeScript functional components, while introducing a typed DeckType enum and adding Prettier as a dev dependency.
Changes:
- Replace
Deck,Board, andMaxLengthTextFieldJS class components with TSX functional equivalents. - Add
DeckTypeenum to the shared frontend types and updateAppto use it when rendering decks. - Add Prettier to
frontenddevDependencies and update lockfile; remove a backend debugSystem.out.println.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/util/MaxLengthTextField.tsx | New TSX functional MaxLengthTextField implementation with typed props and input normalization. |
| frontend/src/util/MaxLengthTextField.js | Removes legacy JS class component version. |
| frontend/src/types/game.ts | Adds DeckType enum for typed deck selection. |
| frontend/src/board/Deck.tsx | New TSX functional Deck with animation logic using hooks/state. |
| frontend/src/board/Deck.js | Removes legacy JS class component version. |
| frontend/src/board/Board.tsx | New TSX functional Board implementation and helpers. |
| frontend/src/board/Board.js | Removes legacy JS class component version. |
| frontend/src/App.tsx | Imports/uses DeckType and applies formatting updates consistent with Prettier-style output. |
| frontend/package.json | Adds Prettier to devDependencies. |
| frontend/package-lock.json | Adds Prettier package resolution/metadata. |
| backend/src/main/java/server/util/GameToJSONConverter.java | Removes debug console output. |
Files not reviewed (1)
- frontend/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,94 @@ | |||
| import React, { Component } from "react"; | |||
There was a problem hiding this comment.
Component is imported but never used in this functional component. This will trigger lint/TS "unused import" warnings; remove it from the import list.
| import React, { Component } from "react"; | |
| import React from "react"; |
| * @effects Calls {@code this.props.onChange} with the modified text of the event, where any whitespace (' ' or '\t') from | ||
| * the front of the text is removed and the text length is trimmed to be {@literal <=} {@code this.props.maxLength} | ||
| */ | ||
| const handleChange = (event: React.ChangeEvent<HTMLInputElement>) => { | ||
| let text = event.target.value; | ||
| text = collapseSpaces(text.trim()); | ||
| if (props.maxLength !== -1) { |
There was a problem hiding this comment.
The comment says only leading whitespace is removed, but text.trim() removes both leading and trailing whitespace. Either update the doc to match the behavior or use trimStart() (or equivalent) if trailing whitespace should be preserved.
| > | ||
| <img | ||
| id="board" | ||
| src={getFascistBoard(props.numFascistPolicies)} |
There was a problem hiding this comment.
getFascistBoard is being called with props.numFascistPolicies, but the helper (and its doc) expects numPlayers. This will select the wrong Fascist board image whenever policy count differs from player count. Pass props.numPlayers instead.
| src={getFascistBoard(props.numFascistPolicies)} | |
| src={getFascistBoard(props.numPlayers)} |
| useEffect(() => { | ||
| if (lastCardCountRef.current === props.cardCount || isPlayingAnimation) { | ||
| return; | ||
| } | ||
| // Number of cards has changed, play an animation. | ||
| setIsPlayingAnimation(true); | ||
| let perCardDelayMs = 150; | ||
| let totalDelayMs = 0; | ||
| const oldCount = lastCardCountRef.current; | ||
| const newCount = props.cardCount; | ||
|
|
||
| if (newCount < oldCount) { | ||
| // Cards were taken out of the deck | ||
| const difference = oldCount - newCount; | ||
| perCardDelayMs = difference <= 3 ? 350 : 150; | ||
| for (let i = oldCount - 1; i >= newCount; i--) { | ||
| setTimeout(() => { | ||
| animateCardVisibility(i, false); | ||
| }, totalDelayMs); | ||
| totalDelayMs += perCardDelayMs; | ||
| } | ||
| } else { | ||
| // Cards were added to the deck | ||
| perCardDelayMs = 150; | ||
| for (let i = oldCount; i < newCount; i++) { | ||
| setTimeout(() => { | ||
| animateCardVisibility(i, true); | ||
| }, totalDelayMs); | ||
| totalDelayMs += perCardDelayMs; | ||
| } | ||
| } | ||
| // Reset animation state after all animations complete. | ||
| setTimeout(() => { | ||
| setIsPlayingAnimation(false); | ||
| }, totalDelayMs); | ||
| lastCardCountRef.current = props.cardCount; |
There was a problem hiding this comment.
This effect schedules multiple setTimeout calls but never cancels them on unmount or when a new animation supersedes an in-flight one. That can cause state updates after unmount (React warning) and unnecessary work. Track timeout IDs (e.g., in a ref) and clear them in the effect cleanup before scheduling new ones.
| <p id={"deck-card-count"}>{props.cardCount}</p> | ||
| <img |
There was a problem hiding this comment.
The displayed card count is now always props.cardCount, so it will jump immediately even though the card hide/show animation is staggered. Previously the UI count updated in sync with each card transition. If that behavior is still desired, keep an internal "animated" count and update it alongside the per-card timeouts.
| <p id={"deck-card-count"}>{props.cardCount}</p> | ||
| <img | ||
| id={"deck-base"} | ||
| src={props.deckType === "DRAW" ? DrawDeck : DiscardDeck} |
There was a problem hiding this comment.
deckType is a DeckType enum, but the render logic compares it to a string literal ("DRAW"). Use DeckType.DRAW/DeckType.DISCARD for type safety and to avoid silent breakage if the enum values change.
| src={props.deckType === "DRAW" ? DrawDeck : DiscardDeck} | |
| src={props.deckType === DeckType.DRAW ? DrawDeck : DiscardDeck} |
Refactors several class components into functional components, and adds Prettier to the project.