Add a way to determine the opened state of a menu

[AdrianGonz97]

Describe what feature you'd like. Pseudo-code, mockups, or screenshots of similar solutions are encouraged!

Previously, there was a way to determine the opened state of a menu by binding to the open prop of the Menu component. Currently, there is no way to easily do that.

One possible solution would be to have the use:menu action dispatch a custom event on the menu element whenever the 'display' style property changes.

What type of pull request would this be?

New Feature

Any links to similar examples or other references we should review?

Here's an example of an event dispatched with the menu action:
https://stackblitz.com/edit/sveltejs-kit-template-default-h26jpx?file=src%2Froutes%2F%2Bpage.svelte,src%2Froutes%2FCurrentMenu.svelte,src%2Froutes%2FModifiedMenu.svelte&terminal=dev

In this example, the menu open state is used to 'highlight' the menu button when the menu is opened.

[endigo9740]

Thanks @AdrianGonz97, expect this in the next release!

[endigo9740]

Hey @AdrianGonz97, so interesting thing with this. Svelte doesn't really seem to have an official recommendation for event handlers within actions. I see in your Stackblitz example you leaned into creating custom JS events and dispatching those. Then listening to those events on the Menu element.

However, when I tried this the project in VS Code Typescript is not happy with this:

Digging around a bit I came across this post on the Svelte Discord support section:
https://discord.com/channels/457912077277855764/1023340103071965194/threads/1023773458183762011

They point to this:
https://github.com/sveltejs/language-tools/blob/master/docs/preprocessors/typescript.md#im-using-an-attributeevent-on-a-dom-element-and-it-throws-a-type-error

Long story short, you essentially have to setup and extend your project to include a custom Type definition to get rid of the warning and red squiggles. Personally I'm not that knowledgeable about Typescript, so when I tried this it wasn't working.

But taking a step back, this seems like a lot of extra hoops to jump through for what we're actually trying to accomplish - which is to say, informing the page of the current menu state. So I've worked up another proof of concept for how we can handle this. Here's how it works:

The Action use directive now includes an addition state key which accepts a function:

<button class="btn btn-ghost" use:menu={{ menu: 'basic', state: stateHandler }}>Basic</button>

Here's an example of my state vars and stateHandler function:

let menuBasicState: boolean = false;
let menuInteractiveState: boolean = false;
	
function stateHandler(res: any): void {
    if (res.menu === 'basic') menuBasicState = res.state;
    if (res.menu === 'interactive') menuInteractiveState = res.state;
}

Internally all the Action is doing is triggering this function like a callback method:

const menuOpen = (): void => {
	elemMenu.style.display = 'block';
	stateEventHandler(true)
}

const menuClose = (): void => {
	elemMenu.style.display = 'none';
	stateEventHandler(false)
}

const stateEventHandler = (state: boolean): void => {
	if (args.state) args.state({menu: args.menu, state});
}

It provides two keys when triggered, the first is the data-menu ID of the menu (ex: "basic" or "interactive" in my example). As well as a state key of TRUE for opened and FALSE for closed. This allows us to have a single state handler function that sorts through and only updates the relevant variable on demand.

Here's what it looks like when displaying the values in the page UI:

Thoughts?

[endigo9740]

I've created a draft pull request here if you would like to try this out:

#340

If you have the GitHub CLI installed you can run a one-line command for this:

gh pr checkout 340

Just start a local server and browse to the Menus documentation page to see this in action.

[AdrianGonz97]

Hi @endigo9740, I've played around with your new version for a bit now and I've even forked it and implemented a version that uses custom events with working typescript support and I think I'm liking your implementation much better!

I really like how everything is defined all together when setting up the action like so:

<span class="relative">
	<button class="btn btn-ghost" use:menu={{ menu: 'basic', state: ({ state }) => (menuBasicState = state) }}>Basic</button>
	<div class="card card-body w-64 shadow-xl" data-menu="basic">
		<p>This menu uses default settings. The position will auto-update depending on the trigger's page location.</p>
	</div>
</span>

rather than having to setup the action on the parent and separately setup the event handler on its child element:

<span class="relative">
	<button class="btn btn-ghost" use:menu={{ menu: 'basic' }}>Basic</button>
	<div class="card card-body w-64 shadow-xl" data-menu="basic" on:menutoggle={(e) => (menuBasicState = e.detail.state)}>
		<p>This menu uses default settings. The position will auto-update depending on the trigger's page location.</p>
	</div>
</span>

Also, having the added benefit of using a single state handler for all of the menus is great too, as you mentioned earlier.

Quick side note, I'm sure you're not fully done with it yet, but I would add this typing for the state function, rather than using the any type in the menu action.

interface ArgsMenu {
	/** The value of 'x' from data-menu-[x] */
	menu: string;
	/** Origin - TRUE: fixed | FALSE: auto */
	fixed?: boolean;
	/** Clicks inside body will not close window */
	interactive?: boolean;
	/** Callback function that informs of current menu state */
	state?: (e: { menu: string; state: boolean }) => void;
}

All in all, this implementation gets a definite +1 from me! 😄

[endigo9740]

@AdrianGonz97 Glad to hear you like the proposal! Yeah having everything tightly coupled in one place is great. Keeps things clean and intuitive, while still providing options.

And yes, absolutely we'll add a well defined type. It's been a hot minute since I setup a Typescript interface for a function and couldn't recall the format! Thanks for the reminder!

I think with that in mind I'll go ahead and push forward with this. I really don't know of a better way to handle this for now. I'll aim to flush this out and document everything asap. Also, I plan to replicate this pattern on the Tooltips, since they operate in a similar manner, because why not. Right?!

It's nearly the end of the day for me on Friday my time, so I'll aim to wrap this up and submit a proper PR. I'd love to get a final review from you before we merge if you are willing and able. No rush though, I won't aim to merge until early next week.

[endigo9740]

@AdrianGonz97 FYI I've pushed the changes mentioned above. This is now present for BOTH the menu and tooltips.

Per the docs, see the section near the bottom of either Usage tab. Likewise this is noted in the Params tab.

Per Tooltips - we don't set data attributes by default, rather it just returns the entire trigger HTMLElement reference. This means you can query it by whatever means you wish, though I've recommended using a data attribute. They are quick and easy and don't interfere with anything!

If all is well I'll aim to merge first thing next week. Thanks for the help on this one!

[endigo9740]

@AdrianGonz97 Per my previous communication this has been merged into the dev branch. This means it will be part of the next release. Thanks!