Svelte: add more general shrinkable path#63770
Conversation
f4731ec to
991d630
Compare
There was a problem hiding this comment.
Rather than creating an empty span below (which complicates the DOM and messes with things like flex spacing), this just inserts an empty text node (a no-op).
991d630 to
1061e7f
Compare
6354af3 to
4af9c77
Compare
There was a problem hiding this comment.
I ran into some issues with the client locking up, and I realized I could limit the events that we respond to quite a bit. Instead of resizing every time anything in the subtree is mutated, we only need to resize when either 1) a direct child node is added or removed, or 2) a direct child node is resized.
This dramatically decreased the number of resize callbacks and made it much easier to reason about resize triggers.
There was a problem hiding this comment.
Apologies for this monstrosity, but in order for highlighting to work correctly, there can be no extra whitespace nodes. Previously, this wasn't a big deal because we used the on:copy hook to remove the added spaces. Unfortunately, even after removing all the whitespace in the DOM, we still need the on:copy hook because the inline-block icon will always add a space when copied, even if it's not in th DOM.
There was a problem hiding this comment.
No change in functionality. Just a small simplification.
There was a problem hiding this comment.
We had this before but it doesn't work.
Seems to be a hidden 'gotcha' with Svelte: You cannot conditionally render slots, but Svelte only warns you about this for elements, not components.
Apparently this is not an issue in Svelte 5 anymore.
| ...baseConfig, | ||
| plugins: [...(baseConfig.plugins || []), 'prettier-plugin-svelte'], | ||
| overrides: [...(baseConfig.overrides || []), { files: '*.svelte', options: { parser: 'svelte' } }], | ||
| overrides: [...(baseConfig.overrides || []), { files: '*.svelte', options: { parser: 'svelte', htmlWhitespaceSensitivity: 'strict' } }], |
There was a problem hiding this comment.
I came across a situation where formatting was adding whitespace in a way I could not get around with comments, so updated our formatter to always respect whitespace rules.
| <!-- | ||
| NOTE: all the weird comments in here are being very careful to not | ||
| introduce any additional whitespace in the path container since that would | ||
| make the path invalid when copied from a selection | ||
| --> |
There was a problem hiding this comment.
Apologies about this. It let me get rid of the "on copy" hook though, and also made it possible to correctly highlight a formatted path. Apparently Svelte 5 is getting saner whitespace rules, so we might be able to revisit this then.
| // HACK: The file icon is placed after the file name in the DOM so it | ||
| // doesn't add any spaces in the file path when copied. This visually | ||
| // reorders the last path element after the file icon. | ||
| .after { | ||
| order: 1; | ||
| } |
There was a problem hiding this comment.
This is how I got around the "inline block svgs add a space to the copied text".
| <!-- | ||
| @component | ||
| ShrinkablePath is a DisplayPath that can collapse its path items | ||
| into a dropdown menu to save space. It does not do this automatically, | ||
| and is usually used alongside a helper like `sizeToFit`. | ||
| --> |
There was a problem hiding this comment.
TIL that @component is needed for this to be treated as a doc comment.
| &:is(:hover, :focus-within) :global([data-copy-button]) { | ||
| opacity: 1; |
There was a problem hiding this comment.
Copy button now shows when the focus is inside the container
| <Tooltip {tooltip} placement="bottom" | ||
| ><slot name="custom" {handleCopy} | ||
| ><Button on:click={handleCopy} variant="icon" size="sm" aria-label={label} | ||
| ><Icon inline icon={ILucideCopy} aria-hidden /></Button | ||
| ></slot | ||
| ></Tooltip | ||
| ></span |
There was a problem hiding this comment.
More space removal. Again, sorry for the ugliness.
There was a problem hiding this comment.
Added a text-only menu item for unlinked dropdown menu.
fkling
left a comment
There was a problem hiding this comment.
See my comments inline, especially about DiffView.
There was a problem hiding this comment.
We had this before but it doesn't work.
Seems to be a hidden 'gotcha' with Svelte: You cannot conditionally render slots, but Svelte only warns you about this for elements, not components.
Apparently this is not an issue in Svelte 5 anymore.
Aah shoot. I always forget that slots cannot be passed in conditionally. I've backed out this change, and added a mental note to check that when messing with slots. Thanks for the catch! |
This adds two new components for the common situation of needing to display a styled path.
DisplayPathhandles splitting, coloring, spacing, slotting in a file icon, adding a copy button, and ensuring that no spaces get introduced when copying the path manuallyShrinkablePathis built on top ofDisplayPathand adds the ability to collapse path elements into a dropdown menuThese are used in three places:
Fixes SRCH-718
Fixes SRCH-690
Video walkthrough
Test plan
Existing tests that I wrote previously cover most of this. I updated them where necessary. Added storybook pages for most of the variants. Lots of manual and visual testing. Interestingly, playwright seems to have simulated copy/paste behavior that does not match real browsers, so I could not rely on that for testing.