Feature/753/new focus unfocus functionality #3706
Conversation
| describe("FocusEffects", () => { | ||
| let actions$: BehaviorSubject<Action> | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| let effects: FocusEffects |
There was a problem hiding this comment.
this variable is unused, can it be removed? (even if not, the comment above it should be)
| this.threeMapControlsService.focusNode(newFocusedNodePath) | ||
| }) | ||
| } else { | ||
| this.threeMapControlsService.focusNode(newFocusedNodePath) |
There was a problem hiding this comment.
This action is performed both inside the if and in the else part of the condition. Can it be moved outside or is that not pissible because its used as a callback in unfocusNode?
There was a problem hiding this comment.
Within the if statement, the callback function is invoked to allow the chaining of the unfocus and focus animations. Without this callback mechanism, only the focus action would need to be triggered directly.
| this.threeMapControlsService.unfocusNode() | ||
| return | ||
| } | ||
| this.threeMapControlsService.unfocusNode() |
There was a problem hiding this comment.
this action is performed both as the first action in and right after the condition. Can it be moved before the condition? this would help with readability
| @@ -94,7 +94,7 @@ export class CodeMapRenderService implements OnDestroy { | |||
| } | |||
|
|
|||
| getVisibleNodes(nodes: Node[]) { | |||
| return nodes.filter(node => node.visible && node.length > 0 && node.width > 0).sort((a, b) => b.height - a.height) | |||
| return nodes.filter(node => node.length > 0 && node.width > 0).sort((a, b) => b.height - a.height) | |||
There was a problem hiding this comment.
i dont know why the part that checks if a node is visible was removed and if that is necessary but if so, we should also rename the function accordingly as it no longer only returns visible nodes
There was a problem hiding this comment.
Checking if a node is visible was a part of the old focus functionality, nodes that have visible attribute set to false won't be rendered. That's why it was removed
| const initialPosition = threeCameraService.camera.position.clone() | ||
|
|
||
| threeMapControlsService.unfocusNode() | ||
| await wait(1100) |
There was a problem hiding this comment.
are wait times this long necessary here? this adds 2 seconds to every time the program is built
There was a problem hiding this comment.
the long waiting time is for the animation to finish
| }) | ||
|
|
||
| it("should not move the camera if positionBeforeFocus is not defined", async () => { | ||
| threeMapControlsService.positionBeforeFocus = undefined |
There was a problem hiding this comment.
This test works even if i remove the line that sets positionBeforeFocus to undefined. Can this case even happen in a production setting?
|
|
#Focus with zoom and camera animation
Issue: #753
Description
Focus Functionality:
Unfocus Functionality:
Camera Animation:
Definition of Done
A PR is only ready for merge once all the following acceptance criteria are fulfilled:
Screenshots or gifs