huntabyte / cmdk-sv

cmdk, but for Svelte ✨
https://cmdk-sv.com
MIT License
414 stars 18 forks source link

Feature request: add `href` prop to `Command.Item` #10

Open shyakadavis opened 8 months ago

shyakadavis commented 8 months ago

Hi;

You know how over at bits's Dropdown Menu you have an optional prop that when passed converts the dropdown item into an anchor tag, well, that would be awesome if Command.Item had it.

Currently, trying to implement a group of nav-links by wrapping a tags has some weird edge cases:

1. The link comes first:

With this implementation:

...
<Command.Group heading="Pages">
        {#each routes as route}
            <a href={route.path} class="inline-flex w-full h-full border">
                <Command.Item class="w-full cursor-pointer" onSelect={command.toggle}>
                    <svelte:component this={route.icon} class="mr-2 h-4 w-4" />
                    <span>{route.name}</span>
                </Command.Item>
            </a>
        {/each}
</Command.Group>
...

The outcome of this is weird, UI-wise. The filtering is messed up, in that the unfiltered links are also rendered, but this time, they are put above the searched item. Take a look at this photo. All those dashes/borders are actual clickable links rendered. But at least, here when I hit enter, it navigates me to my desired page. At least that works.

Screenshot 2023-10-28 at 18 41 01

2. The Command.Item comes first:

With this implementation:

        <Command.Group heading="Pages">
            {#each routes as route}
                <Command.Item onSelect={command.toggle}>
                    <a href={route.path} class="inline-flex w-full h-full border">
                        <svelte:component this={route.icon} class="mr-2 h-4 w-4" />
                        <span>{route.name}</span>
                    </a>
                </Command.Item>
            {/each}
        </Command.Group>

The outcome is also not as expected:

  1. When I search for a link/page, and hit enter, it doesn't navigate me to the link I wanted. It just fires my toggle function, which basically closes the dialog, but I'm stuck on the page I was on.
  2. I put bordered on the links for visual clarity, but I wanted to highlight that the space/field of trigger is affected by the padding of the Item. I know I can fix this but removing it, but I wanted to emphasize that having just the href prop could save everyone the trouble.
Screenshot 2023-10-28 at 18 48 26

What do you think @huntabyte ? Does this sound like a good idea?

huntabyte commented 8 months ago

I don't think it's necessary.

You should be able to do something like:

onSelect={() => {
  goto('/wherever')
})

I need to look further into the implications before determining if it's okay to just convert it into an a tag. Same for the dropdown in bits tbh!

shyakadavis commented 8 months ago

You should be able to do something like:

Awesome. Thanks.

Quick question: This way, is it "okay", in that, shouldn't something that behaves like a link be a link? Or is it still accessible just fine, regardless?

Also, I know this isn't the place, but do you have an idea on how I can navigate to an external URL, with the usual blank target, 'noopener noreferrer' e.t.c using the above workaround?

Doing this: window.open(url, '_blank', 'noopener noreferrer'); will trigger some safety mechanisms (e.g. preventing opening) in browsers like Firefox, and in others, will open a whole new window, yet I just want a new tab. Any ideas?

Update: I have this:

    function navigate_to_external_page(url: string) {
        command.toggle();
        let link = document.createElement('a');
        link.href = url;
        link.target = '_blank';
        link.rel = 'noopener noreferrer';
        link.click();
    }

Is this something you would consider to be safe? Regardless, Firefox still complains, and you first need to confirm, which is a good thing, I guess, ergo my question if this is safe.


I need to look further into the implications before determining if it's okay to just convert it into an a tag. Same for the dropdown in bits tbh!

Good luck. But imo, at least on bit's side, it's nice having the option.

huntabyte commented 8 months ago

I need to confirm later once I'm home, but I think the inverse may be true accessibility wise. Since 'link's should be able to be focused via tab, etc. Maybe a different aria attribute could be added to say it's a navigation item.

As for external links, use window.location = 'link_here'. (https://kit.svelte.dev/docs/modules#$app-navigation-goto)

shyakadavis commented 8 months ago

I need to confirm later once I'm home

Sure. Thanks.

As for external links, use window.location = 'link_here'. (kit.svelte.dev/docs/modules#$app-navigation-goto)

That was the first place I looked. But sadly, it opens the link from the current tab. Basically replaces. But what I want is a separate tab opened.

huntabyte commented 8 months ago

Does this work in place of the window.location method to accomplish what you're looking for @shyakadavis ?

window.open("https://cmdk-sv.com/", "_blank");

shyakadavis commented 8 months ago

Hey, @huntabyte

Does this work in place of the window.location method to accomplish what you're looking for @shyakadavis ?

Nope. It's the same as https://github.com/huntabyte/cmdk-sv/issues/10#issuecomment-1783892871. Right now, I'm settling for that hack above.

If you're still considering the request, I'll leave this open. Otherwise, feel free to close it.

Thanks again.

lukeed commented 7 months ago

For a workaround I would just add asChild to Command.Item & then copy/paste the classnames from Command.Item to your child element.

Slightly repetitive but allows you to use a real anchor tag w/ href to maintain the expected browser behaviors. Eg, I dont think the above suggestions will allow you to CMD/Ctrl click a Command.Item to open a link in a new tab. The above code is hardcoded to always (or not) open in a new tab, since onSelect doesnt have access to the mouse event and on:click doesnt work

shyakadavis commented 7 months ago

Hi, @lukeed

Thanks for the suggestion. And yes, doing "cmd + click" didn't open in a new tab.

I think you're super busy, but do you mind providing a snippet of how you got it to work?

I tried doing that, but quickly ran into some issues, an example being that if I scroll with my keyboard, and hit "enter", it does nothing, perhaps to not having an onSelect, right? But it does help when using the mouse.

This is how far I got.

<Command.Item asChild={true}>
    <a
        href={route.path}
        class={cn(
            'relative flex cursor-default select-none items-center rounded-sm px-2 py-1.5 text-sm outline-none aria-selected:bg-accent aria-selected:text-accent-foreground data-[disabled]:pointer-events-none data-[disabled]:opacity-50'
        )}
    >
        <svelte:component this={route.icon} class={cn('mr-2 !h-[1.1rem] !w-[1.1rem]', route.color)} />
        <span>{route.name}</span>
    </a>
</Command.Item>

Was very curious how you managed to do it, or if it's me who didn't understand your suggestion.

Thanks for the consideration.

niemyjski commented 5 months ago

I'm also interested in this.

vpusher commented 1 month ago

This is definitely a must since it would allow plenty a accessibility features, mobile features (like long press link for preview or copy in clipboard), navigation features like cmd + click. On top, using an anchor is semantically more correct in Web standards than redirecting with JS.