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

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

Open
shyakadavis opened this issue Oct 28, 2023 · 11 comments
Open

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

shyakadavis opened this issue Oct 28, 2023 · 11 comments

Comments

@shyakadavis
Copy link

shyakadavis commented Oct 28, 2023

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
Copy link
Owner

huntabyte commented Oct 28, 2023

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
Copy link
Author

shyakadavis commented Oct 28, 2023

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
Copy link
Owner

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
Copy link
Author

shyakadavis commented Oct 28, 2023

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
Copy link
Owner

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
Copy link
Author

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 #10 (comment). 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
Copy link

lukeed commented Dec 6, 2023

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
Copy link
Author

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
Copy link

I'm also interested in this.

@vpusher
Copy link

vpusher commented Jun 7, 2024

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.

@shyakadavis
Copy link
Author

Found myself wanting this again today, and realized something I missed from Luke's comment. (Back then, I didn't know what asChild or builders did, 😅)

Basically, you need to access the action and other attrs and apply them to the a tag.

Something like:

<Command.Item value={event.title} asChild let:action let:attrs>
	<a
		use:action
		{...attrs}
		href={`/${event.id}/${current_path}`}
		on:click={() => {
			selected_event = event;
			close_and_refocus_trigger(ids.trigger);
		}}
		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'
		)}
	>
		<Icons.Archive class="mr-2 size-4 text-muted-foreground" />
		{event.title}
		<Icons.Check
			class={cn(
				'ml-auto size-4',
				selected_event?.id !== event.id && 'text-transparent'
			)}
		/>
	</a>
</Command.Item>

Though, I'll point out, doing this has some weird edge cases, and I can't confirm search fully works, because I only did this inside a combo box with ≤3 items.

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

No branches or pull requests

5 participants