feat: add command menu for navigation

ieedan commented 2 months ago

Super cool project completely blowing mine out of the water!

I was checking it out on mobile and noticed this wasn't implemented yet.

This is a pretty rough implementation but I think it looks decent and does everything Vercel's does. Unfortunately I can't make use of cmdk-sv here because it only works as a modal not with responsive transformation to a drawer. May be worth opening a PR for in the future.

I had to add a magnifying-glass icon not sure if you are just copy pasting from Vercel but thats what I did.

I put the Command component in the header to save some complexity of having to setup state to call it but it could be moved if needed (would just add some complexity).

github-actions[bot] commented 2 months ago

built with Refined Cloudflare Pages Action

⚡ Cloudflare Pages Deployment

Name	Status	Preview	Last Commit
geist	✅ Ready (View Log)	Visit Preview	bb5600742c04cec3b9519d607cc3c12c8af808aa

ieedan commented 2 months ago

Not sure whats up with the preview doesn't seem to have any of my changes

shyakadavis commented 2 months ago

Hello, @ieedan

Thank you very much for the P.R. and the kind words! Your project is inspiring and sets a high standard.

Regarding the previews not working, I first noticed it recently, even yesterday in #15. The general assumption is that something changed in either GH Actions or Cloudflare, affecting Adrian's custom action setup for Cloudflare Pages. Please bear with this as we await a resolution.

Thanks again for your contribution. Please give me a few hours (I'm about to commute) and I'll provide a proper review.

ieedan commented 2 months ago

I'll be more available in a few hours and I'll take another look thanks!

ieedan commented 2 months ago

I am going to work on this now.

Is auto closing on selection not the intended behavior? Thats normally my default and what Vercel is doing here.

shyakadavis commented 2 months ago

Is auto closing on selection not the intended behavior?

It is. But oddly, it's not working. 🤷

ieedan commented 2 months ago

Is auto closing on selection not the intended behavior?

It is. But oddly, it's not working. 🤷

Yeah sorry I am fixing that right now.

ieedan commented 2 months ago

Everything should be working good now.

I had to create a context for the close function since I separated the list so I added a helper for that.

ieedan commented 2 months ago

Still uses dialog implementation right now but I can change to the cmdk-sv implementation if needed.

ieedan commented 2 months ago

I do think it may still be necessary to have logic for keyboard handling even for 'mobile' users. Sometimes if you are browsing with a smaller screen on a laptop or something it may still be nice to have.

shyakadavis commented 2 months ago

Hey, @ieedan

Could you merge the changes from main and see if the previews are working? (You are working off main on your fork, so I can't commit directly to this P.R.)

ieedan commented 2 months ago

(You are working off main on your fork, so I can't commit directly to this P.R.)

Oops next time I'll make a branch

ieedan commented 2 months ago

Looks like its working TY Adrian

ieedan commented 2 months ago

Not sure if this is a safari thing but there are double arrows on the details component on mobile

ieedan commented 2 months ago

That fix works Claude is awesome man

shyakadavis commented 2 months ago

Hello, @ieedan

That fix works Claude is awesome man

It's also my go-to tool instead of the others 😅

I really wish you could create a new branch off of this one and open a separate P.R, because I figured out a way to simplify things using only cmdk-sv, and I didn't want to discard your input.

Please have a look at the https://github.com/shyakadavis/geist/tree/dev branch, because I feel like this is the best approach forward. Correct me if I'm wrong.

ieedan commented 2 months ago

Yeah that makes more sense. I hadn't tried adding it like that. Let me go ahead and do it that way and open a separate PR unless you think it makes more sense to just use what you already have.

shyakadavis commented 2 months ago

Let's go with your initial draft. If need be, I'll commit with improvements. What do you think?

ieedan commented 2 months ago

I don't want to merge something and make you do more work if you give me a few minutes I can refactor with cmdk-sv

ieedan commented 2 months ago

Sorry this took MUCH longer than I thought it would. Styling was a little difficult then I had a meeting and floor issue I had to address.

shyakadavis commented 2 months ago

Hey, @ieedan

Thanks again for taking the time. I am happy with the current implementation, and all that's left to do is some clean-up.

Namely:

cmd + k not triggering the command menu (tested on Firefox)
ghost class names that render no output
snake_casing 😅
double focus traps when trying to reach esc button
using components like Button instead of raw ones
unused files
...

I just tested to see if I could commit to this PR directly, as opposed to merging and opening a follow-up P.R, and surprisingly, it works, save for having to manually trigger workflows to run.

I don't want to take up more of your time, nor hijack your P.R, but with your permission, I would like to take it across the finish line.

What do you think?

ieedan commented 2 months ago

If that's what you'd prefer. I'm sorry for wasting so much of your time with this 😅.

I noticed the snake case thing I am just so used to camel for JS (even though snake case is better). Maybe worth adding eslint to check that I think there are ways you can enforce it.

I still have time to work on this so I can try and tackle things in the next hour? Still don't want to waste any more of your time so if you'd rather just do it I understand completely.

shyakadavis commented 2 months ago

Please don’t worry about wasting my time—every contribution is greatly appreciated.

Regarding casing, I'll definitely set up ESLint tomorrow.

If you still have time and energy to work on this, please go ahead!

ieedan commented 2 months ago

Issue Tracker

[x] CTRL + K Firefox
[x] Ghost class names
[x] Double focus traps
[x] No more raw dog buttons
[x] Unused files
[x] Snake case

ieedan commented 2 months ago

Okay I think that is everything.

FF is so dumb sometimes I swear why do I have to stopProp and preventDef

ieedan commented 2 months ago

It'll probably be worth squashing this one for a cleaner history.

Sorry about that 😅

shyakadavis commented 2 months ago

These tweaks are in the spirit of keeping the ui components intact in case we wish to re-use them elsewhere. As such, specific overrides ought to happen at the consumer's end, rather than the component definition itself.

As for focus and cmd+k, here is a video of what I was seeing before.

https://github.com/user-attachments/assets/6c935006-35a9-4279-a3b5-80433d1417f5

Should be resolved on my end, would appreciate confirmation that I didn't mess up anything on your end.

I'm really sorry for dragging this out longer than it should have. 🙂

shyakadavis / geist