Fix SelectField MenuOption icons and non-collapsing toggle button.

[willnationsdev]

Related to #132.

[changeset-bot]

🦋 Changeset detected

Latest commit: 0fbcdc1

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
svelte-ux	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[techniq]

Interesting. Not sure why Cloudflare isn't producing a PR preview deployment for this, but they show up for other PRs.

[techniq]

Well crap...

[techniq]

@willnationsdev I re-setup Vercel deployments (and switch to the SvelteKit auto adapter). Can you sync this branch with main and push to hopefully trigger a PR preview (not a major issue for this PR, and I would think would be fixed for new PRs opened). Going to do the same for LayerChart later

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
svelte-ux	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 13, 2024 0:36am

[techniq]

This PR actually wasn't a regression but an improvement :). With that said, could you update the changeset?

Also, you use update this example (with a different name) to leverage options with icon set. I think we won't need to ever set the option slot anymore...

We might need to adjust passing classes to MenuItem to style the icon, but I'm not overly concerned with that tbh (you can still override the slot for these more "complex" use cases). I just thought it would be nice to have an example using option.icon set to show the simplicity of setting these icons.

Thanks!

[willnationsdev]

Gotcha. Makes sense now that I think about it since you never actually deployed, so there's no "regression" to fix. I updated the changeset accordingly.

I also updated the example in the docs so that it shows how to individually control both the text & icon color within both the field & the menu items. I also noticed, in the process, that the way I was passing the fieldClasses into the TextField was incorrect (I now understand that cls(...) only knows how to build up a single class string, not how to merge objects that track multiple class strings - that's what classes is for).

I also modified the description of this PR so that it would close the relevant issue (since it now includes changes that fulfill the remaining tasks in there).

[willnationsdev]

Also, I'm not really sure, but is the placements example at the bottom of the SelectField docs page working correctly? The placement of top-start isn't really cluing me in to whether or not it's doing something cause it seems to more or less behave like the other simple examples. Wondering if merging from the SelectList caused any issues there.

[techniq]

Also, I'm not really sure, but is the placements example at the bottom of the SelectField docs page working correctly? The placement of top-start isn't really cluing me in to whether or not it's doing something cause it seems to more or less behave like the other simple examples. Wondering if merging from the SelectList caused any issues there.

Good catch, but it wasn't a regression with your changes :). Here is a preview before the merge for reference.

I think the issue is I changed autoPlacement to true by default, which picks the side of the viewport with the most space. placement is mostly a suggestion and based on which floating-ui modifiers are enabled determine the final location.

Regarding the button toggle, I see you currently ignore the click if press it, which I think feels wrong (and should toggle close).

CleanShot.2023-12-20.at.09.58.02.mp4

It also reveals an odd issue I've seen that I think is a Svelte bug (hoping Svelte 5 just fixes it, but haven't had time to test). I think the issue is initial prop values are not set when something transitions in/out quickly (at least that's ben the best I could figure out. Basically MenuItem's default classes are being lost (not the font size change in the video).

What if we don't worry about this toggle issue ATM and ship the other improvements, and we can circle back later. If you're OK with that, would you mind backing our those changes (including the new bind:this props on Button). If you still want to try to get this resolved, I'm fine with that too, but I didn't want to hold up getting you a release out.

[willnationsdev]

Regarding the button toggle, I see you currently ignore the click if press it, which I think feels wrong (and should toggle close).

The issue is that the toggle button triggers and I flip the open state accordingly, but then the onClick and onFocus handlers for the TextField execute afterward and end up forcing it back to open = true. If it were just a click handler alone, I could use stopPropagation() to prevent the event from making it to the onClick event, but I couldn't figure out how to make the toggle button prevent other kinds of events from being triggered on the ancestor; so instead, I borrowed from the strategy you were already employing in other parts of the code and directly filtered out the toggle button's elements from being a possible relatedTarget of the MouseEvent or FocusEvent.

What if we don't worry about this toggle issue ATM and ship the other improvements, and we can circle back later. If you're OK with that, would you mind backing our those changes (including the new bind:this props on Button). If you still want to try to get this resolved, I'm fine with that too, but I didn't want to hold up getting you a release out.

That's fine too. I've pushed an update that removes those changes, but I also have a local backup if you end up wanting it restored. Although, if you have another solution in mind that I didn't think of, then I'm all ears since I'm still pretty intermediate-level when it comes to frontend web development.

[techniq]

I appreciate you taking a stab at it, and that might be an approach we can leverage. I recall taking a few attempts at it, hoping to leverage stopPropagation / stopImmediatePropagation, but the focus changing in/out made that difficult IIRC.

I might be able to use some kind of setTimeout() "cooloff" to handle the toggling, similar to how Tooltip changes the delay based on any if Tooltip was shown in the last 500ms (not exactly the same thing or how we would handle it, but a possible approach).

I think for the time being I'd rather leave it as is (a little simpler) until I have a little more bandwidth to really think about it, if you're OK with that :).

Also, I just realized I already have most of this tracked in #91 and #93, so if you're happy, I'm good with merging this PR and closing #132, and tracking those issues separately.

[willnationsdev]

Yeah, that sounds like a good plan to me. Thank you so much for being responsive/active/advisory with regards to all these changes. :-D

[willnationsdev]

@techniq I actually came up with a much smoother & simpler solution that solves the button toggle in a similar vein as before, but without being quite so picky about the elements involved or having to extract elements from other components. Check it out, and let me know if you'd like me to add it to this PR.

[techniq]

@willnationsdev Welcome, I try my best :)

I like that approach much better, although it looks like it still would cancel the closing of the menu if clicked on the toggle icon/button, and I feel that icon should always toggle open or close based on the current state.

Btw, Popover has a similar "clickOutside" check based on 2 elements (anchor/trigger element, and the popover itself). I first thought I had a clickOutside action, but remembered it typically needed more than a single element to check, which is the typical use case for the action. We could still create an action and pass a secondary element for reference, if we decided to implement it at some point. I know clickOutside was discussed being added directly to Svelte in an old RFC, but it kind of hit a block.
Swyx did create a repo of some, including this.

With that said, do you think clicking the toggle icon/button should still open/close based on the current state?

[willnationsdev]

I like that approach much better, although it looks like it still would cancel the closing of the menu if clicked on the toggle icon/button, and I feel that icon should always toggle open or close based on the current state.

With that said, do you think clicking the toggle icon/button should still open/close based on the current state?

It still opens/closes based on the current state. The problem was that when clicking on the toggle button to close, the onBlur event would trigger before the click handler executed. It would set open to false preemptively just beforehand, so that when the toggle click event handler runs, it "flips" it back to being open.

Although, now I'm finding that the blur event is never properly hiding the menu if you tab away from the SelectField, so some adjustment is still necessary, even if it's moving in a better direction. Something about detecting which area was clicked on & then leveraging that.

[techniq]

@willnationsdev I forget where we left this one exactly. I believe you were investigating the chevron clicking issue and improving that interaction. Once #167 is merged (hopefully in the next 2 weeks), could you rebase this PR and we'll see what remains? Thanks!

[willnationsdev]

Yeah, sounds good. I'll do that.

[techniq]

@willnationsdev next has now been merged so you can rebase when you're ready. Thanks!

[willnationsdev]

@willnationsdev next has now been merged so you can rebase when you're ready. Thanks!

@techniq Okay, will do. Might be later tonight, but I'll do it ASAP.

[techniq]

No rush :)

[techniq]

Thanks @willnationsdev. I ran pnpm format locally and pushed to your branch to fix the build complaining. Hopefully the .editorconfig I pushed last night fixes your local Windows environment. Let me know if not and I'll take another look.

I did a quick comparison and noticed the menu items are no longer receiving their text-sm class...

Before	After

Related note: I need to take a good hard look at how we apply class/classes across all components with have a simple/repeatable process (without introducing any regressions). I started tracking this in #245 for reference.

[techniq]

@willnationsdev It's because of how classes is set as the default value if the prop is not defined. Hmm... might have to think about #245 sooner than I hoped :)

[techniq]

@willnationsdev Going to close this for now, but feel free to re-open