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

New variant for tags-mode #78

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ringvold
Copy link

@ringvold ringvold commented Aug 16, 2024

Hi! 😄

This is a suggestion for a new variant of the tag mode. The multiple_select option for the new tags_mode attribute makes it possible to select multiple options quickly without needing to search or force the dropdown to appear. The current tag mode hides the dropdown-part when a option is selected. This approach could probably be described as a kind hybrid of the normal live_select tag mode and a normal multi select input.

A new attribute tags_mode is added with :default (how tags work today) and :multiple_select as possible options.

Example usage taken from demo-app:

<.live_select
   field={my_form[:city_search]}
   selected_option_class="cursor-pointer font-bold hover:bg-gray-400 rounded"
   mode={:tags}
   tags_mode={:multiple_select}
   placeholder="Search for a city"
   update_min_len={3} />

There is some code in the demo app related to selected_option_class as this feature would need some new style defaults/style changes to look passable out of the box. This should probably be solved some other way. Maybe by adding some new default styles.

The name chosen, multiple_select, is very much up for debate. I just needed something to work with.

Potential performance issue

When testing in the demo app I do notice some slowness when selecting or deselecting many options in rapid succession. I did not experience this in our app. I am not sure if this is a flaw in the implementation or just that the dataset for the demo app is very large. Any suggestions on this point is especially welcome.

Breaking changes

The feature also changes the value exposed to the option-slot to also return an indication if the option is already selected. This is one of the main points of the change as the impetus for this feature is a design/UX that displays the selected with a checkbox. See attached picture for example. I suppose this could be done with just styles but the design seems harder to replicate that way and I also think it would be nice to be able to have that info available to the slot. But still open for debate. :)

Example of option-slot usage:
Notice the :let now exposing a tuple which contains the option and a selected boolean.

<:option :let={{%{label: label, value: value}, selected}}>
  <div class="flex justify-content items-center">
    <input
      class="rounded w-4 h-4 mr-3 border border-border"
      type="checkbox"
      checked={selected}
    />
    <span class="text-sm"><%= label %></span>
  </div>
</:option>

Rendered example:
image

This is a suggestion based on functionality that we where in need of and I hope it can be added to the project in some form.

This is not expected to be merged as-is (ex. tests is missing for now) but it is working code that we would use in our fork and a good point to discuss from I think.

I'm aware that this might not be something that this project wants and I'm happy to make changes or discuss the approach. Maybe there is a better way to achive the same or something very similar?

The changes necessary does not seem to invasive though from my perspective (except maybe the option-slot breaking change 😅).

Let me know what you think. 😄

EDIT: I forgot while writing this, but there is also a bug in this implementation I have not figured out. When making the first selection the blur event triggers and hides the dropdown. This does not happen on other selections but consistently happens from state of nothing selected to first option selected.

@ringvold ringvold changed the title Add variant for tags-mode New variant for tags-mode Aug 16, 2024
The multiple select option for the new tags_mode makes it possible to select
multiple options at once without needing to search or force the dropdown
to appear. It is a kind of hybrid of the search variant and a normal
multi select input input.
@maxmarcon
Copy link
Owner

Hi @ringvold thanks a lot for this, I just took a superficial look but it sounds like solid work 💪 using checkboxes in the dropdown is something I've been thinking of adding for a long time now.

Unfortunately, I'm quite busy this coming week, but I will take a proper look as soon as I find some time. Cheers!

@maxmarcon
Copy link
Owner

maxmarcon commented Aug 29, 2024

Hi @ringvold I finally found the time to read carefully what you wrote (I've been really busy in the past few weeks, still busy but things are getting a bit better).

I haven't looked at the code yet. Here's a couple of comments:

  1. I like the idea of enabling a "quick selection mode". I'm not sure I would add a new attribute for it though. To me, it feels cleaner and less clunky to have this as a third mode: :single, :tags and (name also totally up for debate), :quick_tags. Let me know what you think

EDIT: thinking about it a bit more, a simple additional boolean attribute :close_dropdown_on_select would also do and be more explicit.

  1. Couldn't the performance issue you were observing be caused by latency? Were you trying out your app locally? Regardless, the demo app has a bit more of 800 cities, and the search is slow indeed. I should probably add an index, right now the search simply traverses the list of cities and collect the matches.

  2. I really like the idea of exposing the selection status to the option slot. However, I don't think that this needs to be a breaking change. Instead of exposing a tuple, we can add a new field to the map:

<:option :let={%{label: label, value: value, selected: selected}}>

In this way, old code that ignores the selected field would still work

  1. it would be great to allow the user to deselect options using the checkboxes in the selection. But not sure right now how to do that, it's just an idea and probably out of the scope of your feature. In any case, exposing the selected status to the slot is probably a good first step in this direction.

Let me know what you think

@maxmarcon maxmarcon added the enhancement New feature or request label Aug 29, 2024
@ringvold
Copy link
Author

ringvold commented Sep 3, 2024

Hi!

Thought I'd get back to you sooner but stuff happened here as well.

  1. I am very open to how the public API should be. I always thought of it as a variant of the tag mode, but I see how it also could be just a configuration for the tag mode from a users perspective. I felt "quick selection" as you named it has a good description but as for how it should be exposed to the user I'm not sure.

Functionally it is a bit more than "don't close the dropdown" (as I made it here at least) as it also enables you to deselect from the drop down which I believe is not possible on "normal" tags mode. I think that might be why I think of it more as a variant of tag mode. You should have the final say on the API but I feel like :close_dropdown_on_select do not fully encapsulate the functionality I had in mind.

So I lean towards :quick_select/:quick_tags or something like that.

  1. Yes, I tried it locally and still experienced slowness. I tested it out again and it is most pronounced when a lot of elements are selected and selecting very quickly. I suspect it might be a combination of processing a large list of options and handling many selections in quick succession. Maybe a debounce of the selection and handle an event with multiple selections solves it, but that sound like it might be a larger refactor.

  2. Yeah, that should work. Is there any way this could interfere with some custom fields a user might have used with the same name?

  3. I'm not sure what you mean by allowing to use checkboxes. I think about them as visual aide. Selection technically happens on interaction with the parent element. Are there any advantages to changing this? As in it could be advantageous to move the click handler to the checkbox? Exposing the selected status enables custom markdown for selected/unselected options as an addition to the css options available today.

Other things of note

I have noticed that this PR breaks the current behavior for tags as you can deselect from the drop down. Which you can not to in the current released version. I have tried to not change the current functionality of tag mode so I'm not sure what happened there. I consider that a bug.

One thing I have not figured out yet (although not look at it very much yet) is how to not loose focus of the element when selecting it when operating it with keyboard only. Currently it resets focus to the search box when selecting. I would like to keep it in the dropdown at/around the selected element to be able to continue the selection. Any thoughts on this?

Do not move focus back to search box on first select.
Also refactor to use more efficient way of checking for already selected.
@maxmarcon
Copy link
Owner

maxmarcon commented Sep 10, 2024

Functionally it is a bit more than "don't close the dropdown" (as I made it here at least) as it also enables you to deselect from the drop down which I believe is not possible on "normal" tags mode. I think that might be why I think of it more as a variant of tag mode. You should have the final say on the API but I feel like :close_dropdown_on_select do not fully encapsulate the functionality I had in mind.

Yeah apologies I hadn't realized you can deselect from the dropdown...

I'm not sure what you mean by allowing to use checkboxes

And this is exactly what I meant by "using checkboxes to deselect".

Ok so I agree with you now, close_dropdown_on_select obviously doesn't cut it. A third selection mode is more appropriate. quick_tags sounds good to me!

Yeah, that should work. Is there any way this could interfere with some custom fields a user might have used with the same name?

You mean custom fields they're passing to the slots? To preempt this, we could simply taise an exception when someone uses the selected key for the options.

I will take a look at the last 2 points you mentioned (breaking current behavior and not losing focus) soon.

But it sounds like at least we agreed on how to expose this new mode in the API and how to pass the selected status in the slots :)

@ringvold
Copy link
Author

Yes, it does sound like we are onto something her 😄

You mean custom fields they're passing to the slots? To preempt this, we could simply raise an exception when someone uses the selected key for the options.

That is maybe also a breaking change but certainly much smaller than my suggestion.

I will make a suggestion on the default styles for this mode. I cant remember the details but there are some changes that need to be made to make it look decent out of the box.

@maxmarcon
Copy link
Owner

That is maybe also a breaking change but certainly much smaller than my suggestion.

I don't think this is a breaking change because we're not breaking any API. The docs say that the options can be specified as maps with the shape %{label: label, value: value}. It doesn't state that other keys in the map will be made available to the slots.

@egeersoz
Copy link

Is this one good to merge?

@maxmarcon
Copy link
Owner

@egeersoz no, we just discussed a few points and I'm sure @ringvold has still a few things to add before it's ready for prime time :)

@ringvold
Copy link
Author

ringvold commented Sep 20, 2024

@egeersoz I still have to implement what me and Max talked about here, add test and then probably a new check by @maxmarcon. I hope to find some time to look at it this weekend though. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants