Open ringvold opened 3 months ago
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!
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:
:single
, :tags
and (name also totally up for debate), :quick_tags
. Let me know what you thinkEDIT: thinking about it a bit more, a simple additional boolean attribute :close_dropdown_on_select
would also do and be more explicit.
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.
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
Let me know what you think
Hi!
Thought I'd get back to you sooner but stuff happened here as well.
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.
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.
Yeah, that should work. Is there any way this could interfere with some custom fields a user might have used with the same name?
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.
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?
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 :)
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.
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.
Is this one good to merge?
@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 :)
@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. :)
Life happens and I have not gotten to this when I thought. ππββ‘οΈ
@maxmarcon Looking a bit at it now and I remember why I did this functionality as a new variant for tags. It is a bit easier to implement when it is just a variant of tags as we can piggyback on the existing tags functionality with some other minor changes. Introducing a new mode we have to make sure not to mess up :tags
and also add :quick_tags
to most, if not all places where we are checking for :tags
. It will probably be fine but I think that is why I defaulted to a new variant for tags.
On another note my fix for the dropdown removal on first select did not work. It basically just removed all phx-focus
use defined externally (parent focus events). π
The fix seems to be to always render the tags container div but conditionally render the rest. (It might be the re rendering when the first tag is added that triggers the blur event that hides the dropdown.) In practice it would be to move the if
on line 11 inside the div beneath it.
The down side of this approach is that the dropdown is not closed when removing the last selected tag which I am now used to, but it probably cant be helped.
Hoping to have something to look at soon.
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:
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.
Rendered example:
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.