Reimplement context menu

[D0m1nos]

This pull request aims to reimplement the context menu removed after the new ui.

Right-clicking on a song in the song list lets you add it to a playlist (will be implemented in #60 ) or put it as the next song in the queue (if the queue exists).

Right-clicking on a song in the queue lets you remove it or add it to a playlist.

[D0m1nos]

I just noticed that if the context menu is too far to the right or too far down it will get clipped. Will try to fix tomorrow.

[brw]

shouldn't this also use the existing Popover component? That should also fix this as that's handled by Floating UI

I just noticed that if the context menu is too far to the right or too far down it will get clipped. Will try to fix tomorrow.

[D0m1nos]

shouldn't this also use the existing Popover component? That should also fix this as that's handled by Floating UI

I've been trying to use the Popover component but it looks like it needs a trigger object (like a button) for it to work properly which doesn't exist because it is triggered by the right click.

I found a hacky way to get around it but the context menu still clips (because i'm still setting the position "manually").

    <Popover isOpen={show} placement="top">
      <Popover.Trigger class="absolute left-0 top-0 bg-accent" />
      <Popover.Overlay />
      <Popover.Content>
        <div
          class={"absolute z-30 rounded-lg bg-thick-material"}
          style={{ top: pos()[1] + "px", left: pos()[0] + "px" }}
          ref={menu}
        >
          <div class="flex flex-col gap-1 rounded-lg bg-thick-material p-2">
            <For each={props.children}>{(child) => child}</For>
          </div>
        </div>
      </Popover.Content>
    </Popover>

Also, the Dropdown component in the settings uses Popover but also clips when the window is too small. Looking at the floating-ui docs it seems like we need to add flip() to the middleware in Popover.tsx and autoUpdate() so it updates when resizing the window.

I'll try looking into using floating-ui directly instead of the Popover component.

[D0m1nos]

After messing around with floating-ui for a bit i found a solution.

First i tried using flip() to flip the context menu when it gets clipped, the problem is that apparently if the coordinates set in the middleware differ too much from the initial ones the flip() middleware doesn't work, it does the calculations but doesn't flip the menu. I tried calculating the new position manually but that caused other issues so instead i used shift().

This clamps the menu position to the side instead of clipping.

It's possible to flip the menu if we want to but it requires a couple of workarounds that could cause more issues and isn't worth it in my opinion.

[duduBTW]

We should use the popover (and maybe also the List) component to avoid difference in styles on the project.

I've been trying to use the Popover component but it looks like it needs a trigger object (like a button) for it to work properly which doesn't exist because it is triggered by the right click.

The idea is having a "more" button on the right of the cards that will show up when hover it:

SongItem:

PlaylistItem:

we don't have this component yet, but it has the same idea

Right clicking will open the popover that is connect to the "more" trigger on the right.

You can add the necessary middlewares to floating-ui on thePopover component so everything works, just make sure it is customizable and it doesn't break the dropdown.

[D0m1nos]

After some tinkering i managed to use the Popover component and should now work properly (at least on linux).

Also i wanted to apologize for not using the correct components/libraries initially (thanks duduBTW, brw and Tnixc for pointing me in the right direction), it's my first time working on a codebase this big so this is all new to me.

[brw]

No need to apologize really! Your efforts are very much appreciated. We want this to be an environment for people to learn new things. There are no stakes, expectations or deadlines here, so don't worry about it honestly

[D0m1nos]

All the issues mentioned should be fixed now. I also fixed the context menu clipping again and added a way to style the context menu items.

Thanks for the encouraging words brw, they really mean a lot to me :)

[brw]

Some things I'm noticing at first glance:

• I don't think the context menu needs that extra 15px horizontal offset, it feels too far away imo. But we can always tweak it later
• I'd expect the context menu to open at the position of the cursor on right click (maybe a little difficult though)
• The trigger button should not disappear while the context menu is open (especially not when it's still being hovered)
• I don't think you want to pass middlewares to the Popover component. Popover should add middlewares based on its props. You're setting the offset twice now for example: once through the offset prop, which adds an offset middleware, and once through another offset middleware.

The styling could use some work too but I suppose we might bring that in line with the actual design later.

[brw]

Oh yea I also don't really like dynamically hiding/showing context menu items, e.g. the Play Next button only showing up when there's a queue. It should probably be disabled instead of hidden

[D0m1nos]

I don't think you want to pass middlewares to the Popover component.

You're right, that's also what the docs say but... (see next point)

I'd expect the context menu to open at the position of the cursor on right click (maybe a little difficult though)

That's what i originally did by adding a custom middleware, i then removed it because dudu said (correct me if i misunderstood) "Right clicking will open the popover that is connect to the "more" trigger on the right."

If the context menu needs to be at the position of the cursor then i need some help implementing it, because i don't know how to do it without passing middlewares directly. Should i add a prop for a custom middleware? Should i add the middleware in the Popover component and a boolean prop to enable it?

Other than that i will implement the style changes mentioned.

[brw]

Should i add the middleware in the Popover component and a boolean prop to enable it

Yea that's what I meant. I think that's the way to do it.

Clicking on the trigger should open the menu belonging to the component at the trigger position.
Right clicking on a component should open the menu belonging to that the component at the cursor position.
Unless @duduBTW thinks differently about it but this seems pretty standard to me.

[D0m1nos]

The context menu now opens on the mouse position when right-clicking and the unclickable items are now disabled instead of hidden.

Question about the trigger button, should it only stay visible when left-clicking it and stay hidden when right-clicking elsewhere? Right now it looks like this when right-clicking near it:

[brw]

Right now it shows on hover and stays visible until the context menu goes away, both for the right-click context menu and left-click trigger menu. I think that's fine.

Spotify for example seems to hide the trigger once you move your cursor into the context menu after right-clicking, but keeps the trigger visible when left-clicking on it until the context menu is dismissed. I guess that's technically better but I don't think it's that big of a deal. So unless other people have different opinions about it I'd say it's good like this

[brw]

Latest changes seem wrong to me. The trigger now always disappears as soon as the menu is opened either through right click or through the trigger.

I think the problem is just with focus. The trigger should always be shown as long as you're hovered over the song, even if the menu is open. And if you want to follow what Spotify does it should also stay shown when moving the cursor into the menu if the menu was opened by clicking on the trigger.

[D0m1nos]

That's weird, it was not what i intended to do and when i tried it the trigger didn't disappear when left clicking.

Anyways, i reverted the change for now, this is only a minor visual concern that i think should be talked about when we will implement the newer UI as it looks fine now.

When the Popover appears it traps all of the window focus in the Popover.Overlay, so if we wanted to mimic Spotify we would have to remove that but then it would cause other issues like:

[brw]

There is probably a good solution for that but yea I guess something to look at later

[Tnixc]

Could we make it so that if you right click on another song while a menu is already open dismiss the currently open the menu and open the menu for that song?
(basically open menu while another menu is open)

[duduBTW]

Could we make it so that if you right click on another song while a menu is already open dismiss the currently open the menu and open the menu for that song? (basically open menu while another menu is open)

I don't think implementing that effort right now, I also like keeping the interactions on the popover consistent (clicking away closes it).

[brw]

Should definitely do that at some point though. It is how people would expect it to work coming from other apps