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

refactor: simplify SVG path filtering and parent selection #899

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

devin-ai-integration[bot]
Copy link
Contributor

@devin-ai-integration devin-ai-integration bot commented Dec 14, 2024

feat: improve SVG element handling

  • Hide all SVG child elements from layers panel (not just paths)
  • Prevent selecting SVG child elements in canvas
  • When clicking SVG child elements, select parent SVG instead
  • Show parent SVG outline when hovering over SVG paths
  • Ensure consistent visual feedback for SVG interactions

These changes improve the SVG handling in Onlook by:

  1. Filtering out all SVG child elements from the layers panel for a cleaner interface
  2. Making SVG selection more intuitive by only allowing parent SVG selection
  3. Automatically selecting parent SVG when clicking any SVG child element
  4. Providing consistent visual feedback by highlighting parent SVGs during hover and selection

Technical improvements:

  • Simplified SVG path filtering by using direct parent-child relationship check (element is path && parent is svg)
  • Consolidated SVG interaction handling in rect.ts for better maintainability

Link to Devin run: https://app.devin.ai/sessions/21294b0db5aa42fa8e249b59a998daa1

Copy link
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR
  • Look at CI failures and help fix them

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Add "(aside)" to your comment to have me ignore it.

- Hide all SVG child elements from layers panel (not just paths)
- Prevent selecting SVG child elements in canvas
- When clicking SVG child elements, select parent SVG instead
@drfarrell
Copy link
Collaborator

(aside) @Kitenite tested this and works really well. The goal here was to make it so we didn't have pesky svg paths that you couldn't actually edit with Onlook in the editor. Looking over the code it seems like the changes work decently, but I'm going to tag it over to you to confirm if there are any major things that we need to improve. Otherwise, happy with the result!

@drfarrell drfarrell requested a review from Kitenite December 14, 2024 04:00
@@ -42,10 +42,17 @@ export function buildLayerTree(root: HTMLElement): Map<string, LayerNode> | null

const layerMap = new Map<string, LayerNode>();
const treeWalker = document.createTreeWalker(root, NodeFilter.SHOW_ELEMENT, {
acceptNode: (node: Node) =>
isValidHtmlElement(node as HTMLElement)
Copy link
Contributor

@Kitenite Kitenite Dec 14, 2024

Choose a reason for hiding this comment

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

You could just update this to check for the parent being svg and node being path

export function isValidHtmlElement(element: Element): boolean {
return (
element &&
element instanceof Node &&
element.nodeType === Node.ELEMENT_NODE &&
!DOM_IGNORE_TAGS.includes(element.tagName) &&
!element.hasAttribute(EditorAttributes.DATA_ONLOOK_IGNORE) &&
(element as HTMLElement).style.display !== 'none'
);
}

@devin-ai-integration devin-ai-integration bot changed the title feat: hide svg paths from layers list refactor: simplify SVG path filtering and parent selection Dec 14, 2024
@drfarrell
Copy link
Collaborator

(aside) @Kitenite I've had it go through a ton of attempts at getting this to work with your suggestion, but it just can't quite seem to get it. This version (0dbfa3b) before your suggestion worked from a UX perspective perfectly, but I know you'd prefer it to also be implemented well.

I think it needs code intervention to make the suggested edit, but the version I linked above was the best UX of any attempts it has made since.

Should I try using Cursor to implement your suggested edit, or you want to take a stab?

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