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.

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

Fixed deleting element around array/map expressions #786

Merged
merged 18 commits into from
Dec 7, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
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
26 changes: 26 additions & 0 deletions apps/studio/electron/main/code/diff/helpers.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import generate, { type GeneratorOptions } from '@babel/generator';
import type { NodePath } from '@babel/traverse';
import * as t from '@babel/types';
import { EditorAttributes } from '@onlook/models/constants';
import type { DynamicType } from '@onlook/models/element';
import { nanoid } from 'nanoid/non-secure';

export function getOidFromJsxElement(element: t.JSXOpeningElement): string | null {
Expand Down Expand Up @@ -79,3 +81,27 @@ export const jsxFilter = (
export function generateCode(ast: t.File, options: GeneratorOptions, codeBlock: string): string {
return generate(ast, options, codeBlock).code;
}

export function getDynamicType(elementPath: NodePath<t.JSXElement>): DynamicType | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

My understanding is that this traverse up the entire element tree, is this too much? I would think if the immediate parent is an array then we will add it, otherwise we don't. In this case, we don't want to delete a but do we delete b and c?

Perhaps once we run into another parent that is a jsxelement, we can return.

<>{arr.map(_ =>
   <a>
     <b/>
     <c/>
   <a/>
)}</>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

export function getDynamicType(elementPath: NodePath<t.JSXElement>): DynamicType | null {
    const parentPath = elementPath.parentPath;
    
    if (t.isJSXElement(parentPath.node)) {
        return null; // Stop if we hit another JSX element
    }


    if (parentPath && t.isCallExpression(parentPath.node) &&
        t.isMemberExpression(parentPath.node.callee) &&
        t.isIdentifier(parentPath.node.callee.property) &&
        parentPath.node.callee.property.name === 'map') {
        return 'array';
    }

    return null;
}

@Kitenite does this solve the concern with returning early if the parent is another jsx element?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Never mind, it didn't work as expected. root element in the array were not flagged as dynamic

let currentPath: NodePath<t.Node> = elementPath;

// Traverse the AST to find detect dynamic elements
while (currentPath.parentPath) {
const parentPath = currentPath.parentPath;
const parentNode = parentPath.node;

// check for array
if (
t.isCallExpression(parentNode) &&
t.isMemberExpression(parentNode.callee) &&
t.isIdentifier(parentNode.callee.property) &&
parentNode.callee.property.name === 'map'
) {
return 'array';
}

currentPath = parentPath;
}

return null;
}
5 changes: 5 additions & 0 deletions apps/studio/electron/main/run/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import * as fs from 'fs';
import { customAlphabet } from 'nanoid/non-secure';
import * as nodePath from 'path';
import { VALID_DATA_ATTR_CHARS } from '/common/helpers/ids';
import { getDynamicType } from '../code/diff/helpers';

export const ALLOWED_EXTENSIONS = ['.jsx', '.tsx'];
export const IGNORED_DIRECTORIES = ['node_modules', 'dist', 'build', '.next', '.git'];
Expand Down Expand Up @@ -68,11 +69,15 @@ export function getTemplateNode(
? getTemplateTag(path.node.closingElement)
: null;
const component = componentStack.length > 0 ? componentStack[componentStack.length - 1] : null;

const dynamicType = getDynamicType(path);

const domNode: TemplateNode = {
path: filename,
startTag,
endTag,
component,
dynamicType,
};
return domNode;
}
Expand Down
9 changes: 8 additions & 1 deletion apps/studio/electron/preload/webview/api.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
import { contextBridge } from 'electron';
import { processDom } from './dom';
import { getDomElementByDomId, getElementAtLoc, updateElementInstance } from './elements';
import { getActionElementByDomId, getActionLocation } from './elements/dom/helpers';
import {
getActionElementByDomId,
getActionLocation,
setDynamicElementType,
getDynamicElementType,
} from './elements/dom/helpers';
import { getInsertLocation } from './elements/dom/insert';
import { getRemoveActionFromDomId } from './elements/dom/remove';
import { getElementIndex } from './elements/move';
Expand All @@ -22,6 +27,8 @@ export function setApi() {
// Elements
getElementAtLoc,
getDomElementByDomId,
setDynamicElementType,
getDynamicElementType,

// Actions
getActionLocation,
Expand Down
22 changes: 22 additions & 0 deletions apps/studio/electron/preload/webview/elements/dom/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { getOrAssignDomId } from '../../ids';
import { getImmediateTextContent } from '../helpers';
import { elementFromDomId } from '/common/helpers';
import { getInstanceId, getOid } from '/common/helpers/ids';
import { EditorAttributes } from '@onlook/models/constants';
import type { DynamicType } from '@onlook/models/element';

export function getActionElementByDomId(domId: string): ActionElement | null {
const el = elementFromDomId(domId);
Expand Down Expand Up @@ -77,3 +79,23 @@ export function getActionLocation(domId: string): ActionLocation | null {
originalIndex: index,
};
}

export function getDynamicElementType(domId: string): DynamicType | null {
const el = document.querySelector(
`[${EditorAttributes.DATA_ONLOOK_DOM_ID}="${domId}"]`,
) as HTMLElement | null;

if (!el) {
console.warn('No element found', { domId });
return null;
}

return el.getAttribute(EditorAttributes.DATA_ONLOOK_DYNAMIC_TYPE) as DynamicType;
}

export function setDynamicElementType(domId: string, dynamicType: string) {
const el = document.querySelector(`[${EditorAttributes.DATA_ONLOOK_DOM_ID}="${domId}"]`);
if (el) {
el.setAttribute(EditorAttributes.DATA_ONLOOK_DYNAMIC_TYPE, dynamicType);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export function getRemoveActionFromDomId(
}

const location = getElementLocation(el);

if (!location) {
console.error('Failed to get location for element:', el);
return;
Expand Down
10 changes: 10 additions & 0 deletions apps/studio/src/lib/editor/engine/ast/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,16 @@ export class AstManager {
return;
}

if (templateNode.dynamicType) {
node.dynamicType = templateNode.dynamicType;
const webview = this.editorEngine.webviews.getWebview(webviewId);
if (webview) {
webview.executeJavaScript(
`window.api?.setDynamicElementType('${node.domId}', '${templateNode.dynamicType}')`,
);
}
}

this.findNodeInstance(webviewId, node, node, templateNode);
}

Expand Down
16 changes: 16 additions & 0 deletions apps/studio/src/lib/editor/engine/element/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type { DomElement } from '@onlook/models/element';
import { debounce } from 'lodash';
import { makeAutoObservable } from 'mobx';
import type { EditorEngine } from '..';
import { toast } from '@onlook/ui/use-toast';

export class ElementManager {
private hoveredElement: DomElement | undefined;
Expand Down Expand Up @@ -155,6 +156,20 @@ export class ElementManager {
return;
}

const dynamicElementType = await webview.executeJavaScript(
`window.api?.getDynamicElementType('${selectedEl.domId}')`,
);

if (dynamicElementType) {
toast({
title: 'Invalid Action',
description: `This element is part of a react expression (${dynamicElementType}) and cannot be deleted`,
variant: 'destructive',
});

return;
}

const removeAction = (await webview.executeJavaScript(
`window.api?.getRemoveActionFromDomId('${selectedEl.domId}', '${webviewId}')`,
)) as RemoveElementAction | null;
Expand All @@ -166,6 +181,7 @@ export class ElementManager {
if (!codeBlock) {
console.error('Code block not found');
}

this.editorEngine.action.run(removeAction);
}
}
1 change: 1 addition & 0 deletions packages/models/src/constants/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export enum EditorAttributes {
DATA_ONLOOK_DRAG_START_POSITION = 'data-onlook-drag-start-position',
DATA_ONLOOK_NEW_INDEX = 'data-onlook-new-index',
DATA_ONLOOK_EDITING_TEXT = 'data-onlook-editing-text',
DATA_ONLOOK_DYNAMIC_TYPE = 'data-onlook-dynamic-type',
}

export enum WebviewChannels {
Expand Down
4 changes: 4 additions & 0 deletions packages/models/src/element/layers.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { z } from 'zod';

export const DynamicTypeEnum = z.enum(['array', 'conditional', 'unknown']);
export type DynamicType = z.infer<typeof DynamicTypeEnum>;

const LayerNodeSchema = z.object({
domId: z.string(),
webviewId: z.string(),
Expand All @@ -8,6 +11,7 @@ const LayerNodeSchema = z.object({
textContent: z.string(),
tagName: z.string(),
isVisible: z.boolean(),
dynamicType: DynamicTypeEnum.optional(),
component: z.string().nullable(),
children: z.array(z.string()).nullable(),
parent: z.string().nullable(),
Expand Down
2 changes: 2 additions & 0 deletions packages/models/src/element/templateNode.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { z } from 'zod';
import { DynamicTypeEnum } from './layers';

export const TemplateTagPositionSchema = z.object({
line: z.number(),
Expand All @@ -15,6 +16,7 @@ export const TemplateNodeSchema = z.object({
startTag: TemplateTagSchema,
endTag: TemplateTagSchema.nullable(),
component: z.string().nullable(),
dynamicType: DynamicTypeEnum.nullable().optional(),
});

export type TemplateNode = z.infer<typeof TemplateNodeSchema>;
Expand Down