Drag and Drop players in the editor #11

Merged
maxime.batista merged 12 commits from editor/place-players into master 1 year ago

This pull request adds a court in the tactic editor, with players that can be drag and dropped to the court.

You can also move the players inside the court by dragging them wherever you want

This pull request adds a court in the tactic editor, with players that can be drag and dropped to the court. You can also move the players inside the court by dragging them wherever you want
maxime.batista added 9 commits 1 year ago
clement.freville2 requested changes 1 year ago
clement.freville2 left a comment

React elements shouldn't be stored in a state. The state represents the data, and a React element represents the rendered view, that isn't properly typed. It also not play well when passing the initial data to the component, and when exposing the state to the consumers. I would highly recommend separating them by mapping the array each time.

React elements shouldn't be stored in a state. The state represents the data, and a React element represents the rendered view, that isn't properly typed. It also not play well when passing the initial data to the component, and when exposing the state to the consumers. I would highly recommend separating them by mapping the array each time.
export interface RackInput {
id: string,
objects: [ReactElement[], Dispatch<SetStateAction<ReactElement[]>>],
-     objects: [ReactElement[], Dispatch<SetStateAction<ReactElement[]>>],
+     objects: RackObject[],
+     onChange: (objects: RackObject[]) => void

The state is internal to the current component, and shouldn't be exposed as it is to its children. Just pass the objects in two separate props: one for the array, and one for a on change handler. The parent component will then have the liberty to handle object changes as it wants.

```diff - objects: [ReactElement[], Dispatch<SetStateAction<ReactElement[]>>], + objects: RackObject[], + onChange: (objects: RackObject[]) => void ``` The state is internal to the current component, and shouldn't be exposed as it is to its children. Just pass the objects in two separate props: one for the array, and one for a *on change* handler. The parent component will then have the liberty to handle object changes as it wants.
maxime.batista marked this conversation as resolved
export interface RackInput {
id: string,
objects: [ReactElement[], Dispatch<SetStateAction<ReactElement[]>>],
canDetach: (ref: RefObject<HTMLDivElement>) => boolean,
-     canDetach: (ref: RefObject<HTMLDivElement>) => boolean,
+     canDetach: (ref: HTMLDivElement) => boolean,

RefObject is not appropriated because it is a nullable internal to the component. Expose directly the HTML element.

```diff - canDetach: (ref: RefObject<HTMLDivElement>) => boolean, + canDetach: (ref: HTMLDivElement) => boolean, ``` `RefObject` is not appropriated because it is a nullable internal to the component. Expose directly the HTML element.
maxime.batista marked this conversation as resolved
useEffect(() => {
const bounds = divRef.current!.getBoundingClientRect();
setCourtPlayers(players.map(player => {

BasketCourt shouldn't have a state if it is always derived from its props.

`BasketCourt` shouldn't have a state if it is always derived from its props.
maxime.batista marked this conversation as resolved
import React, {useRef} from "react";
- import React, {useRef} from "react"; 
+ import {useRef} from "react"; 

React doesn't need to be in scope.

```diff - import React, {useRef} from "react"; + import {useRef} from "react"; ``` React doesn't need to be in scope.
maxime.batista marked this conversation as resolved
import Draggable, {DraggableBounds} from "react-draggable";
import {PlayerPiece} from "./PlayerPiece";
export interface PlayerOptions {
- export interface PlayerOptions {
+ export interface CourtPlayerProps { 

React calls them properties.

```diff - export interface PlayerOptions { + export interface CourtPlayerProps { ``` React calls them *properties*.
maxime.batista marked this conversation as resolved
handle={".player-piece"}
nodeRef={ref}
bounds={bounds}
defaultPosition={{x: x, y: y}}
-             defaultPosition={{x: x, y: y}} 
+             defaultPosition={{x, y}} 
```diff - defaultPosition={{x: x, y: y}} + defaultPosition={{x, y}} ```
maxime.batista marked this conversation as resolved
/**
* Percentage of the player's position to the bottom (0 means top, 1 means bottom, 0.5 means middle)
*/
bottom_percentage: number
-     bottom_percentage: number 
+     bottomRatio: number,

Ratio is more appropriated for a value between 0 and 1.

```diff - bottom_percentage: number + bottomRatio: number, ``` Ratio is more appropriated for a value between 0 and 1.
maxime.batista marked this conversation as resolved
const positions = ["1", "2", "3", "4", "5"]
const [allies, setAllies] = useState(
positions.map(pos => <PlayerPiece team="allies" key={pos} text={pos}/>)
-         positions.map(pos => <PlayerPiece team="allies" key={pos} text={pos}/>) 
+         [...positions]

The state shouldn't contain React elements.

```diff - positions.map(pos => <PlayerPiece team="allies" key={pos} text={pos}/>) + [...positions] ``` The state shouldn't contain React elements.
maxime.batista marked this conversation as resolved
maxime.batista force-pushed editor/place-players from 0e23e8706b to ef80aa3192 1 year ago
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is passing Details
a6cceb31ea
apply suggestions
maxime.batista force-pushed editor/place-players from a6cceb31ea to 170dd58a99 1 year ago
maxime.batista force-pushed editor/place-players from 170dd58a99 to f58c85ac0a 1 year ago
maxime.batista requested review from yanis.dahmane-bounoua 1 year ago
maxime.batista requested review from vivien.dufour 1 year ago
maxime.batista requested review from mael.daim 1 year ago
maxime.batista requested review from samuel.berion 1 year ago
maxime.batista requested review from clement.freville2 1 year ago
maxime.batista force-pushed editor/place-players from b213729e50 to c53a1b024c 1 year ago
clement.freville2 approved these changes 1 year ago
clement.freville2 left a comment

Great!

Great!
}
export function BasketCourt({players, onPlayerRemove}: BasketCourtProps) {
const divRef = useRef<HTMLDivElement>(null);
-     const divRef = useRef<HTMLDivElement>(null); 

Unused reference

```diff - const divRef = useRef<HTMLDivElement>(null); ``` Unused reference
* */
export default function CourtPlayer({player, onRemove}: PlayerProps) {
const ref = useRef<HTMLDivElement>(null);
-    const ref = useRef<HTMLDivElement>(null); 

Unused reference

```diff - const ref = useRef<HTMLDivElement>(null); ``` Unused reference
import React, {CSSProperties, useState} from "react";
import {CSSProperties, ReactElement, RefObject, useRef, useState} from "react";
- import {CSSProperties, ReactElement, RefObject, useRef, useState} from "react"; 
+ import {CSSProperties, useRef, useState} from "react"; 

Unused imports

```diff - import {CSSProperties, ReactElement, RefObject, useRef, useState} from "react"; + import {CSSProperties, useRef, useState} from "react"; ``` Unused imports
vivien.dufour approved these changes 1 year ago
samuel.berion approved these changes 1 year ago
samuel.berion approved these changes 1 year ago
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is passing Details
a18014d4c3
apply suggestions
maxime.batista merged commit 8483ef08dd into master 1 year ago
maxime.batista deleted branch editor/place-players 1 year ago

Reviewers

mael.daim was requested for review 1 year ago
yanis.dahmane-bounoua was requested for review 1 year ago
clement.freville2 approved these changes 1 year ago
vivien.dufour approved these changes 1 year ago
samuel.berion approved these changes 1 year ago
continuous-integration/drone/push Build is passing
The pull request has been merged as 8483ef08dd.
Sign in to join this conversation.
No Milestone
No project
No Assignees
4 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: IQBall/Application-Web#11
Loading…
There is no content yet.