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

Conversation

iNerdStack
Copy link
Contributor

Description

What is the purpose of this pull request?

  1. Fixed issue with deleting elements around array/map expressions
  2. Added flag to non-static elements to prevent delete action
  • New feature
  • Documentation update
  • Bug fix
  • Refactor
  • Release
  • Other

Related: #644

Copy link
Contributor

@Kitenite Kitenite left a comment

Choose a reason for hiding this comment

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

Some comments on the signature. This is an amazing solution and I think just the flagging can solve a lot of headache!

const signature = [
child.tagName,
child.className,
child.getAttribute(EditorAttributes.DATA_ONLOOK_ID),
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: We're making an assumption that if an element has the same data-onlook-id as its sibling, it would be part of an array?

One edge case is if an element is an instance and it's used twice, these would have the same signature?

export default function Page() {
    return (
        <div>
            <Instance />
            <Instance />
        </div>
    );
}

function Instance() {
    return <div>Instance</div>;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Thought this is really close! I wonder if there's something we can do on the plugin side to populate these attributes

apps/studio/electron/preload/webview/dom.ts Outdated Show resolved Hide resolved
apps/studio/electron/preload/webview/dom.ts Outdated Show resolved Hide resolved
@Kitenite
Copy link
Contributor

Kitenite commented Dec 2, 2024

@iNerdStack , does this need review?

@iNerdStack
Copy link
Contributor Author

@Kitenite No not yet, I have some pending local changes to move dynamic type logic and also alerts when attempting to delete element in an array etc, i will make the push soon and inform you

@Kitenite
Copy link
Contributor

Kitenite commented Dec 3, 2024

@iNerdStack awesome, just wanted to make sure I'm not blocking. Take your time :)

@iNerdStack
Copy link
Contributor Author

@Kitenite
The PR is tested and ready for review

@@ -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

Copy link
Contributor

@Kitenite Kitenite left a comment

Choose a reason for hiding this comment

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

This is awesome! I added a stack to track only the parent's dynamic type. Tested with nested maps and it works really well too!

@Kitenite Kitenite merged commit 2e40d6a into onlook-dev:main Dec 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants