IQBall
/
Application-Web
6
0
Fork 0
Code Issues 49 Pull Requests Packages Projects 5 Releases Wiki Activity
Labels Milestones

Add screen phantoms #113

Merged
maxime.batista merged 7 commits from editor/screen-phantoms into master 1 year ago
Conversation 0 Commits 7 Files Changed 15
maxime.batista commented 1 year ago

Add phantoms when doing a screen to a player (or another phantom !).

image

Add phantoms when doing a screen to a player (or another phantom !). ![image](/attachments/5d7aff87-6317-4a76-9eb9-3671865837b0)
image.png
37 KiB
maxime.batista added 5 commits 1 year ago
continuous-integration/drone/push Build is passing Details
75028fb6b2 format
maxime.batista requested review from clement.freville2 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 added 5 commits 1 year ago
continuous-integration/drone/push Build is passing Details
5359bb12de format
clement.freville2 requested changes 1 year ago
src/components/editor/BasketCourt.tsx Outdated
setForceEmptyComponents(false)
}, [setForceEmptyComponents])
const usedComponents = forceEmptyComponents ? [] : components
clement.freville2 commented 1 year ago 
-    const [forceEmptyComponents, setForceEmptyComponents] = useState(true)
-
-    useLayoutEffect(() => {
-        setForceEmptyComponents(false)
-    }, [setForceEmptyComponents])
-
-    const usedComponents = forceEmptyComponents ? [] : components
+    const usedComponents = components

You may only use useLayoutEffect to access browser APIs that aren't available during the first render. It is not the case here.

```diff - const [forceEmptyComponents, setForceEmptyComponents] = useState(true) - - useLayoutEffect(() => { - setForceEmptyComponents(false) - }, [setForceEmptyComponents]) - - const usedComponents = forceEmptyComponents ? [] : components + const usedComponents = components ``` You may only use `useLayoutEffect` to access browser APIs that aren't available during the first render. It is not the case here.
maxime.batista marked this conversation as resolved
src/editor/PlayerDomains.ts Outdated
): PlayerLike | undefined {
const playerOrigin =
player.type === "player" ? player : getOrigin(player, components)
const pathItems = playerOrigin.path?.items!
clement.freville2 commented 1 year ago 
-    const pathItems = playerOrigin.path?.items!
+    if (!playerOrigin.path) return undefined
+    const pathItems = playerOrigin.path.items!

Optional chain expressions can return undefined by design, but you immediately use a non-null assertion.
no-non-null-asserted-optional-chain

```diff - const pathItems = playerOrigin.path?.items! + if (!playerOrigin.path) return undefined + const pathItems = playerOrigin.path.items! ``` Optional chain expressions can return undefined by design, but you immediately use a non-null assertion. [no-non-null-asserted-optional-chain](https://typescript-eslint.io/rules/no-non-null-asserted-optional-chain/)
maxime.batista marked this conversation as resolved
src/editor/PlayerDomains.ts Outdated
targetIdx == 0
? playerOrigin
: getComponent<PlayerLike>(pathItems[targetIdx - 1], components)
return result
clement.freville2 commented 1 year ago 
-    const result =
-        targetIdx == 0
-            ? playerOrigin
-            : getComponent<PlayerLike>(pathItems[targetIdx - 1], components)
-    return result
+    return targetIdx == 0
+        ? playerOrigin
+        : getComponent<PlayerLike>(pathItems[targetIdx - 1], components)
```diff - const result = - targetIdx == 0 - ? playerOrigin - : getComponent<PlayerLike>(pathItems[targetIdx - 1], components) - return result + return targetIdx == 0 + ? playerOrigin + : getComponent<PlayerLike>(pathItems[targetIdx - 1], components) ```
maxime.batista marked this conversation as resolved
src/editor/PlayerDomains.ts Outdated
content = updateComponent(
{
...playerBefore,
actions: playerBefore.actions.toSpliced(actionIdx, 1),
clement.freville2 commented 1 year ago 
-            const actionIdx = playerBefore.actions.findIndex(
-                (a) => a.target === pos.attach,
+            const actions = playerBefore.actions.filter(
+                (a) => a.target !== pos.attach,
             )
             content = updateComponent(
                 {
                     ...playerBefore,
-                    actions: playerBefore.actions.toSpliced(actionIdx, 1),
+                    actions,
```diff - const actionIdx = playerBefore.actions.findIndex( - (a) => a.target === pos.attach, + const actions = playerBefore.actions.filter( + (a) => a.target !== pos.attach, ) content = updateComponent( { ...playerBefore, - actions: playerBefore.actions.toSpliced(actionIdx, 1), + actions, ```
maxime.batista marked this conversation as resolved
src/editor/PlayerDomains.ts Outdated
const origin = getOrigin(player, content.components)
return truncatePlayerPath(origin, player, content)
content = truncatePlayerPath(origin, player, content)
console.log(content)
clement.freville2 commented 1 year ago 
-        console.log(content)
```diff - console.log(content) ```
maxime.batista marked this conversation as resolved
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is passing Details
c6ca4e6c76 apply suggestions
maxime.batista force-pushed editor/screen-phantoms from c6ca4e6c76 to a316edc341 1 year ago
maxime.batista requested review from clement.freville2 1 year ago
maxime.batista force-pushed editor/screen-phantoms from a316edc341 to 1043207e2d 1 year ago
clement.freville2 requested changes 1 year ago
src/editor/PlayerDomains.ts Outdated
? document
.getElementById(lastSegmentStart)!
.getBoundingClientRect()
: lastSegmentStart
clement.freville2 commented 1 year ago

Are we really sure that heavily nested ternaries is the way to go?

Are we really sure that heavily nested ternaries is the way to go?
maxime.batista commented 1 year ago
Poster

yeah i also find it ugly but TS does not treat blocks and condition expressions as value expressions 😔
maybe i should use declare the variable ahead and use if/else to initialize it

yeah i also find it ugly but TS does not treat blocks and condition expressions as value expressions 😔 maybe i should use declare the variable ahead and use if/else to initialize it
maxime.batista marked this conversation as resolved
src/editor/PlayerDomains.ts Outdated
id: string,
components: TacticComponent[],
): T {
return components.find((c) => c.id === id)! as T
clement.freville2 commented 1 year ago

Unsafe cast incoming...

Unsafe cast incoming...
maxime.batista commented 1 year ago
Poster

It is more of an utility function than an actual feature/trait definition function

I used to write components.find((c) => c.id === id)! as T anyway so there is no changes with this function.

But i think there is a way to prove that the component is actually of the expected type, just didn't took the time to look at it.

I think a cast is relevant here as we search a component by and identifier and there is no such case where two TacticComponent can have the same identifier but with a different type

It is more of an utility function than an actual feature/trait definition function I used to write `components.find((c) => c.id === id)! as T` anyway so there is no changes with this function. But i think there is a way to prove that the component is actually of the expected type, just didn't took the time to look at it. I think a cast is relevant here as we search a component by and identifier and there is no such case where two TacticComponent can have the same identifier but with a different type
clement.freville2 commented 1 year ago

I quite agree that in some scenarios, runtime tricks cannot be easily avoided and may be fine.

The unsafe taint was previously "delimited" because it was written explicitly. This convenience function hides it. IDs may be inadvertently swapped/copy pasted.

Anyway, this code interface cannot be made more type safe without changing some of the infrastructure.

It could, maybe, be useful to check the type at runtime:

function getComponent<T extends TacticComponent>(
    id: string,
    components: TacticComponent[],
    check: (c: TacticComponent) => c is T,
): T {
    return components.find((c) => c.id === id && check(c)) as T
}

// Export pre-confonfigured "searchers"
export function getPlayerComponent(id: string, components: TacticComponent[]): Player {
    return getComponent(id, components, (c): c is Player => c.type === "player")
}
I quite agree that in some scenarios, runtime tricks cannot be easily avoided and may be fine. The unsafe taint was previously "delimited" because it was written explicitly. This convenience function hides it. IDs may be inadvertently swapped/copy pasted. Anyway, this code interface cannot be made more type safe without changing some of the infrastructure. It could, maybe, be useful to check the type at runtime: ```ts function getComponent<T extends TacticComponent>( id: string, components: TacticComponent[], check: (c: TacticComponent) => c is T, ): T { return components.find((c) => c.id === id && check(c)) as T } // Export pre-confonfigured "searchers" export function getPlayerComponent(id: string, components: TacticComponent[]): Player { return getComponent(id, components, (c): c is Player => c.type === "player") } ```
src/model/tactic/Player.ts Outdated
export interface Player extends PlayerInfo, Component<"player"> {
export interface Player extends PlayerInfo, Component<"player", Pos> {
readonly id: PlayerId
}
clement.freville2 commented 1 year ago 
-
-export interface Player extends PlayerInfo, Component<"player", Pos> {
-    readonly id: PlayerId
-}

Two Player types are exported.

```diff - -export interface Player extends PlayerInfo, Component<"player", Pos> { - readonly id: PlayerId -} ``` Two `Player` types are exported.
maxime.batista marked this conversation as resolved
maxime.batista added 1 commit 1 year ago
continuous-integration/drone/push Build is passing Details
2a5de88380 Apply suggestions
maxime.batista requested review from clement.freville2 1 year ago
clement.freville2 approved these changes 1 year ago
maxime.batista merged commit fd9b5e2063 into master 1 year ago
maxime.batista deleted branch editor/screen-phantoms 1 year ago

Reviewers

yanis.dahmane-bounoua was requested for review 1 year ago
mael.daim was requested for review 1 year ago
vivien.dufour was requested for review 1 year ago
samuel.berion was requested for review 1 year ago
clement.freville2 approved these changes 1 year ago
continuous-integration/drone/push Build is passing
Details
The pull request has been merged as fd9b5e2063.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
yanis.dahmane-bounoua
mael.daim
vivien.dufour
samuel.berion
clement.freville2
Labels
Apply labels
Clear labels
bug
Something is not working duplicate
This issue or pull request already exists enhancement
New feature help wanted
Need some help invalid
Something is wrong question
More information is needed wontfix
This won't be fixed
No Label bug duplicate enhancement help wanted invalid question wontfix
Milestone
Set milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Assign users
Clear assignees
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: IQBall/Application-Web#113
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'editor/screen-phantoms'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes