Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve drag previews #152

Closed
wants to merge 4 commits into from
Closed

Improve drag previews #152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
97ac900
add custom preview drag image
fredjens Dec 15, 2020
165bf92
update example
fredjens Dec 15, 2020
8e80ae8
update docs
fredjens Dec 15, 2020
9ee387b
update example to https
fredjens Dec 15, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 26 additions & 3 deletions packages/core/src/events/DefaultEventHandlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,20 +141,43 @@ export class DefaultEventHandlers extends CoreEventHandlers {
],
},
create: {
init: (el) => {
init: (el, _, previewImageUrl) => {
el.setAttribute('draggable', 'true');

if (previewImageUrl) {
// Preload preview image
const preImg = document.createElement('link');
preImg.href = previewImageUrl;
preImg.rel = 'preload';
preImg.as = 'image';
document.head.appendChild(preImg);
}

return () => el.removeAttribute('draggable');
},
events: [
defineEventListener(
'dragstart',
(e: CraftDOMEvent<DragEvent>, userElement: React.ReactElement) => {
(
e: CraftDOMEvent<DragEvent>,
userElement: React.ReactElement,
previewImageUrl?: string
) => {
e.craft.stopPropagation();
const tree = this.store.query
.parseReactElement(userElement)
.toNodeTree();

DefaultEventHandlers.draggedElementShadow = createShadow(e);
if (previewImageUrl) {
// Custom drag image

const previewImage = new Image();
previewImage.src = previewImageUrl;
e.dataTransfer.setDragImage(previewImage, 0, 0);
} else {
DefaultEventHandlers.draggedElementShadow = createShadow(e);
}

DefaultEventHandlers.draggedElement = tree;
}
),
Expand Down
18 changes: 11 additions & 7 deletions packages/core/src/utils/Handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export type CraftDOMEvent<T extends Event> = T & {

export type CraftEventListener = [
string,
(e: CraftDOMEvent<Event>, opts: any) => void,
(e: CraftDOMEvent<Event>, opts: any, previewImage?: string) => void,
boolean
];

Expand All @@ -26,12 +26,13 @@ export type Handler = {
* The DOM manipulations to perform on the attached DOM element
* @returns function that reverts the DOM manipulations performed
*/
init: (el: HTMLElement, opts: any) => any;
init: (el: HTMLElement, opts: any, previewImage?: string) => any;

/**
* List of Event Listeners to add to the attached DOM element
*/
events: readonly CraftEventListener[];
previewImageUrl?: string;
Copy link
Owner

@prevwong prevwong Jan 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fredjens for this PR, I really like this one!

I'm just not too sure about adding previewImage directly into the internal API of the event handlers. However, this is the fault of the current state of the internal handlers, which definitely requires some refactoring.

My original idea for the API of the connectors was as such:

connectors.connectorName<T>(dom: HTMLElement, opts: T)

Where opts could be an object that we can pass to the connector. But it just so happens that almost every connectors' opts happened to just be a single value which is nodeId most of the time (ie: connectors.drag(dom, nodeId).

Anyways, I'm thinking we could do either one of the following to fix this situation:

Solution 1: Add a 3rd parameter for additional options

In this case, we basically turn the 2nd parameter to contain the required value by the connector (ie: the nodeId) and we add a 3rd parameter for additional options

connectors.connectorName<R, T>(dom: HTMLElement, nodeId: R, moreOptions?: Partial<T>)

// Hence:
connector.drag(dom, nodeId) // or
connector.drag(dom, nodeId, { previewImage })

connector.create(dom, <UserComponent />) // or
connector.create(dom, <UserComponent />, { previewImage })

Solution 2: Create an alias for the 2nd parameter

In this case, the connectors' 2nd parameter can either be the single required value (ie: nodeId) or an object to specify required value and additional options.

connectors.connectorName<R>(dom: HTMLELement, nodeId: R);
connectors.connectorName<R, T>(dom: HTMLELement, { nodeId, previewImage }: {nodeId: R} & Partial<T>); 

// Hence:
connector.drag(dom, nodeId) // or
connector.drag(dom, {nodeId, previewImage })

connector.create(dom, <UserComponent />) // or
connector.create(dom,  { element: <UserComponent />, previewImage })

Personally, I prefer the first solution because it's probably easier to achieve; and the only benefit of the 2nd solution is that we keep parameter count to just 2, but at the expense of added complexity. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer solution 1, too.

I don't think the added complexity is worth it. And I would argue, that from an API consumer's perspective (esp. beginners)

// this is more readable
connectors.connectorName<R, T>(dom: HTMLElement, nodeId: R, moreOptions?: Partial<T>)

// than the "overload" version
connectors.connectorName<R>(dom: HTMLELement, nodeId: R);
connectors.connectorName<R, T>(dom: HTMLELement, { nodeId, previewImage }: {nodeId: R} & Partial<T>);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi there! Thanks for the feedback! Really sorry, but I do not think I have time to finish this in the nearest future... So if anyone else want to finish this, feel free!

};

export type ConnectorsForHandlers<T extends Handlers> = ReturnType<
Expand Down Expand Up @@ -64,6 +65,7 @@ const isEventBlockedByDescendant = (e, eventName, el) => {
class WatchHandler {
el: HTMLElement;
opts: any;
previewImage?: string;

private handler: Handler;
private unsubscribe: () => void;
Expand All @@ -74,6 +76,7 @@ class WatchHandler {
this.el = el;
this.opts = opts;
this.handler = handler;
this.previewImage = undefined;

this.unsubscribe = store.subscribe(
(state) => ({ enabled: state.options.enabled }),
Expand All @@ -94,9 +97,10 @@ class WatchHandler {
}

private add() {
const { init, events } = this.handler;
const { init, events, previewImageUrl } = this.handler;

this.cleanDOM = init && init(this.el, this.opts, previewImageUrl);

this.cleanDOM = init && init(this.el, this.opts);
this.listenersToRemove =
events &&
events.map(([eventName, listener, capture]) => {
Expand All @@ -117,8 +121,7 @@ class WatchHandler {

e.craft.blockedEvents[eventName].push(this.el);
};

listener(e, this.opts);
listener(e, this.opts, previewImageUrl);
}
};

Expand Down Expand Up @@ -172,7 +175,7 @@ export abstract class Handlers<T extends string = null> {
return accum;
}

const connector = (el, opts) => {
const connector = (el, opts, previewImageUrl?: string) => {
if (!el || !document.body.contains(el)) {
this.wm.delete(el);
return;
Expand All @@ -189,6 +192,7 @@ export abstract class Handlers<T extends string = null> {
[key]: new WatchHandler(this.store, el, opts, {
init,
events,
previewImageUrl,
}),
});
};
Expand Down
2 changes: 1 addition & 1 deletion packages/docs/docs/api/useEditor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ const { connectors, actions, query, ...collected } = useEditor(collector);
["select", "(dom: HTMLElement, nodeId: NodeId) => HTMLElement", "Specifies the DOM that when clicked will in turn click the specified Node's user component"],
["hover", "(dom: HTMLElement, nodeId: NodeId) => HTMLElement", "Specifies the DOM that when hovered will in turn hover the specified Node's user component"],
["drag", "(dom: HTMLElement, nodeId: NodeId) => HTMLElement", "Specifies the DOM that when dragged will move the specified Node's user component. Only applicable if the component is rendered as an immediate child of a &lt;Canvas /&gt; component."],
["create", "(dom: HTMLElement, userElement: React.ReactElement) => HTMLElement", "Specifies the DOM that when dragged will create a new instance of the specified User Element at the drop location."]
["create", "(dom: HTMLElement, userElement: React.ReactElement, previewImageUrl: string) => HTMLElement", "Specifies the DOM that when dragged will create a new instance of the specified User Element at the drop location."]
]],
["actions", "ActionMethods", [
["add", "(nodes: Node, parentId?: NodeId, index?: number) => void", "Add a Node to the given parent node ID at the specified index. By default the parentId is the id of the Root Node"],
Expand Down
14 changes: 14 additions & 0 deletions packages/examples/basic/components/Toolbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,20 @@ export const Toolbox = () => {
Card
</MaterialButton>
</Grid>
<Grid container direction="column" item>
<MaterialButton
ref={(ref) =>
connectors.create(
ref,
<Button text="Click me" size="small" />,
'https://placekitten.com/200/200'
)
}
variant="contained"
>
Customized drag preview
</MaterialButton>
</Grid>
</Grid>
</Box>
);
Expand Down
9 changes: 5 additions & 4 deletions packages/utils/src/wrapConnectorHooks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,13 +53,13 @@ function throwIfCompositeComponentElement(element: React.ReactElement<any>) {
}

export function wrapHookToRecognizeElement(
hook: (node: any, opts: any) => void
hook: (node: any, opts: any, previewImage?: string) => void
) {
return (elementOrNode = null, opts: any) => {
return (elementOrNode = null, opts: any, previewImage: string) => {
// When passed a node, call the hook straight away.
if (!isValidElement(elementOrNode)) {
const node = elementOrNode;
node && hook(node, opts);
node && hook(node, opts, previewImage);
return node;
}

Expand All @@ -81,5 +81,6 @@ export type ConnectableElement =

export type Connector = (
elementOrNode: ConnectableElement,
options?: any
options?: any,
previewImage?: string
) => React.ReactElement | null;