Update modal component branch #4109

Caleb-T-Owens · 2024-06-18T12:34:56Z

No description provided.

Caleb-T-Owens · 2024-06-19T09:25:19Z

app/src/lib/branches/dragActions.ts

+class BranchDragActions {
+	constructor(
+		private branchController: BranchController,
+		private branch: Branch
+	) {}


I was thinking about whether I should have some

interface DropzoneActions { accepts: //... onDrop: //... }

So we'd have one class for each dropzone actions. It would also mean that we could pass something that implements that interface directly to the Dropzone component, but I didn't quite want to go that far yet.

Yeah, makes sense. Definitely sounds like a good next step. However as you said, lets maybe save that for a more full-scale refactor of the whole drag-n-drop system 👍

Caleb-T-Owens · 2024-06-19T09:26:27Z

app/src/lib/branches/dragActions.ts

+export class BranchDragActionsFactory {
+	constructor(private branchController: BranchController) {}
+
+	build(branch: Branch) {
+		return new BranchDragActions(this.branchController, branch);
+	}
+}


I'm using a factory so that when we go to use the drag actions, we only need to provide the relevant state, rather than having to also provide a bunch of controllers and servicies

Caleb-T-Owens · 2024-06-19T09:27:36Z

app/src/lib/components/BranchDropzone.svelte

+<div class="canvas-dropzone">
+	<Dropzone {accepts} ondrop={onDrop}>
+		{#snippet overlay({ hovered, activated })}
+			<div class="new-virtual-branch" class:activated class:hovered>
+				<div class="new-virtual-branch__content">
+					<div class="stimg">
+						<div class="stimg__hand">
+							{@html handSvg}
+						</div>
+						<div class="stimg__top-sheet">


Because this is quite a specialised dropzone, I chose not to have some generic component for the overlay that it uses.

Caleb-T-Owens · 2024-06-19T09:28:05Z

app/src/lib/components/BranchLanePopupMenu.svelte

@@ -159,7 +159,7 @@
 			kind="solid"
 			on:click={async () => {
 				await branchController.deleteBranch(branch.id);
-				deleteBranchModal.close();
+				close();


😁 a cheeky unrelated change... if only we could work in multiple branches at the same time

Caleb-T-Owens · 2024-06-19T09:29:37Z

app/src/lib/components/CommitDragItem.svelte

+	const actions = $derived.by(() => {
 		if (!$branch) return;

-		if (commit instanceof RemoteCommit) {
-			return () => false;
-		}
-		return (data: DraggableCommit) => {
-			if (data.commit.isParentOf(commit)) {
-				branchController.squashBranchCommit(data.branchId, commit.id);
-			} else if (commit.isParentOf(data.commit)) {
-				branchController.squashBranchCommit(data.branchId, data.commit.id);
-			}
-		};
-	}
+		return commitDragActionsFactory.build($branch, commit);
+	});


I was considering saying something like $derived($branch && commitDragActionsFactory.build($branch, commit)), but I decided that I really just want to keep everything as simple as possible, and being explicit and having that sort of guard clause to me just expressed the intent much better

Yup, makes it more readable too, etc. etc. Good call 👍

Caleb-T-Owens · 2024-06-19T09:31:11Z

app/src/lib/components/CommitDragItem.svelte

+		{#if actions}
+			{@render ammendDropzone()}
+		{:else}
+			{@render children()}
+		{/if}


This component needs to gracefully fall back to not talking to the branchController because its used in the remoteBranch and trunk pages.

I thought about having the snippets inside the if block too, but it ended up looking too messy

Caleb-T-Owens · 2024-06-19T09:31:41Z

app/src/lib/components/CommitDragItem.svelte

+{#snippet ammendDropzone()}
+	<Dropzone accepts={actions!.acceptAmend.bind(actions)} ondrop={actions!.onAmend.bind(actions)}>
+		{@render squashDropzone()}
+
+		{#snippet overlay({ hovered, activated })}
+			<CardOverlay {hovered} {activated} label="Ammend commit" />
+		{/snippet}
+	</Dropzone>
+{/snippet}
+
+{#snippet squashDropzone()}
+	<Dropzone accepts={actions!.acceptSquash.bind(actions)} ondrop={actions!.onSquash.bind(actions)}>
+		{@render children()}
+
+		{#snippet overlay({ hovered, activated })}
+			<CardOverlay {hovered} {activated} label="Squash commit" />
+		{/snippet}
+	</Dropzone>
+{/snippet}


If anyone has n idea about how to better express the nesting, I am all ears

Caleb-T-Owens · 2024-06-19T09:32:41Z

app/src/lib/components/CommitList.svelte

+	function getReorderDropzoneOffset({
+		isFirst = false,
+		isMiddle = false,
+		isLast = false
+	}: {
+		isFirst?: boolean;
+		isMiddle?: boolean;
+		isLast?: boolean;
+	}) {
+		if (isFirst) return 12;
+		if (isMiddle) return 6;
+		if (isLast) return 0;
+		return 0;
+	}
 </script>

+{#snippet reorderDropzone(dropzone: ReorderDropzone, yOffsetPx: number)}
+	<Dropzone accepts={dropzone.accepts.bind(dropzone)} ondrop={dropzone.onDrop.bind(dropzone)}>
+		{#snippet overlay({ hovered, activated })}
+			<LineOverlay {hovered} {activated} {yOffsetPx} />
+		{/snippet}
+	</Dropzone>
+{/snippet}


I didn't think an entire component was required for the Reordering dropzone. I'm willing to be swayed on this position though.

Caleb-T-Owens · 2024-06-19T09:46:32Z

app/src/lib/dragging/dropzone.ts

+		this.registeredOnDrop = async (e: DragEvent) => await this.onDrop(e, data);
+		this.registeredOnDragEnter = this.onDragEnter.bind(this);
+		this.registeredOnDragLeave = this.onDragLeave.bind(this);


I do make the assumption that the user can't drag two things at once, but that seems like a pretty sound assumption to me

Caleb-T-Owens · 2024-06-19T09:47:36Z

app/src/lib/dragging/dropzone.ts

+
+export const dropzoneRegistry = new Map<HTMLElement, Dropzone>();
+
+export function dropzone(node: HTMLElement, opts: DropzoneConfiguration) {


This is now only consumed by the Dropzone, so its not relevant to support a partial configuration that contains defaults

Caleb-T-Owens · 2024-06-19T10:03:28Z

app/src/lib/dragging/reorderDropzoneManager.ts

+}
+
+// Private classes used to calculate distances between commtis
+class Indexer {


I decided to move the indexing and distance calculations into their own classes, because its now on longer required to be publicly exported (like it was when we used the old ReorderDropzone.svelte component)

ndom91

LGTM functionality-wise!

I have a few small niggles design-wise, but you mentioned you still need to talk to Pavel so I'll just hold off 😊

Caleb-T-Owens · 2024-06-19T13:56:50Z

Waiting on sveltejs/svelte-eslint-parser#540 in order for our linting to support nested snippets.

asdfasdf

Caleb-T-Owens force-pushed the update-modal-component-branch branch 3 times, most recently from 44e43b9 to b41b27d Compare June 18, 2024 16:26

Caleb-T-Owens requested a review from ndom91 June 19, 2024 09:08

Caleb-T-Owens commented Jun 19, 2024

View reviewed changes

Caleb-T-Owens marked this pull request as ready for review June 19, 2024 10:03

Caleb-T-Owens force-pushed the update-modal-component-branch branch 2 times, most recently from 4728b8e to 976e0a1 Compare June 19, 2024 10:41

ndom91 approved these changes Jun 19, 2024

View reviewed changes

github-actions bot added the svelte label Jun 19, 2024

Caleb-T-Owens added 9 commits June 21, 2024 09:45

Drag and drop

5756aea

asdfasdf

Refactor dropzones

2aad52d

Rename some files and start to use context to inject classes

a0e9e2b

Refactor code into ts files

80209b8

Enough factories to make the once-ler happy

ad03baf

Refactor dropzone manager

541e6a9

Add in a full width option

ba48fc6

Fix update phase of lifecycle

4bdba5c

tweak styles

f1fe727

Caleb-T-Owens added 2 commits June 21, 2024 09:45

Fix eslint errors

89af946

Remove rebase runaway

95e6b14

Caleb-T-Owens force-pushed the update-modal-component-branch branch from f755df4 to 95e6b14 Compare June 21, 2024 07:51

Style tweak

e068f2d

Caleb-T-Owens merged commit ffaffe6 into master Jun 21, 2024
16 checks passed

Caleb-T-Owens deleted the update-modal-component-branch branch June 21, 2024 07:55


		export const dropzoneRegistry = new Map<HTMLElement, Dropzone>();

		export function dropzone(node: HTMLElement, opts: DropzoneConfiguration) {

Update modal component branch #4109

Update modal component branch #4109

Uh oh!

Conversation

Caleb-T-Owens commented Jun 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ndom91 left a comment

Choose a reason for hiding this comment

Uh oh!

Caleb-T-Owens commented Jun 19, 2024

Uh oh!

Uh oh!

Uh oh!