-
Notifications
You must be signed in to change notification settings - Fork 58
Allow individual options to be disabled #110
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
Allow individual options to be disabled #110
Conversation
2b894be
to
bad266d
Compare
Allows LiveSelect to have it's options disabled in single or tag mode. This way it can act similar to a regular select with some options disabled. In Tag or Quick Tag mode, the options stay disabled even after removing a selection after hitting the maximum items selected. This also slightly refactors `normalize_option/1` to have function heads for tuples, maps, lists and other (atoms or binary). I ended up running into the credo complexity warning due to the extra tuple case that I added. So I thought this would be a fair compromise to resolve any potential complexity in the function.
bad266d
to
2d9bfc1
Compare
I was running into the CI failing for a credo complexity check. So I opted to simplify the logic by breaking it up into different function heads. There is a function head for maps, lists, tuples and a final one to handle atoms or binary values.
This modifies the behavior in Tags and Single mode for clicking a disabled object. Previously clicking one in either mode would close the dropdown, but leave the user focused in the input. This potentially gave an appearance to the User that their input was accepted. So instead of closing the option dropdown, it's left open for the user to interact with like a regular select would with disabled options.
The styling option `unavailable_class` was not set in the module attribute `@styling_options` so it was never declared. Anytime a LiveSelect had this attribute passed in, it threw a warning.
2d9bfc1
to
2c16488
Compare
Thank you! I will review as soon as I find some time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is solid work. Thank you. I left a few questions and suggestions.
) | ||
when max_selectable > 0 and length(selection) >= max_selectable do | ||
assign(socket, hide_dropdown: not quick_tags_mode?(socket)) | ||
socket |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why did we remove this logic? Isn't it needed for quick_tags
mode?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't affect the quick_tags
mode as the drop down should stay visible to the user. Though the change does affect tags
, changing it's behavior at max selection to match quick_tags
.
For both the socket
should be returned unchanged so the dropdown isn't closed when clicking on an option after hitting the max selectable items. This way in tags
mode it doesn't give the appearance of accepting the user's input by hiding the dropdown, when really it was ignored and discarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can revert this change as well if it's a behavior you don't want to change within tags
.
if Keyword.keyword?(option) do | ||
Map.new(option) | ||
|> normalize_option() | ||
else | ||
:error | ||
end | ||
end | ||
|
||
defp normalize_option(option) when is_map(option) do | ||
case option do | ||
%{key: key, value: _value} = option -> | ||
{:ok, Map.put_new(option, :label, key)} | ||
{:ok, Enum.into(option, %{label: key, disabled: false})} | ||
|
||
%{value: value} = option -> | ||
{:ok, Map.put_new(option, :label, value)} | ||
{:ok, Enum.into(option, %{label: value, disabled: false})} | ||
|
||
option when is_list(option) -> | ||
if Keyword.keyword?(option) do | ||
Map.new(option) | ||
|> normalize_option() | ||
else | ||
:error | ||
end | ||
_ -> | ||
:error | ||
end | ||
end | ||
|
||
defp normalize_option(option) when is_tuple(option) do | ||
case option do | ||
{label, value} -> | ||
{:ok, %{label: label, value: value}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice refactoring!
test/live_select_tags_test.exs
Outdated
|
||
type(live, "ABC") | ||
select_nth_option(live, 1, method: :click) | ||
assert_selected_multiple_static(live, [%{value: 2, label: "B"}]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can use assert_selected_multiple
here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I've updated it to use assert_selected_multiple/2
here.
|
||
describe "Disabled option tests" do | ||
test "tuples options can be disabled", %{conn: conn} do | ||
stub_options([{"A", 1, false}, {"B", 2, true}, {"C", 3, false}]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this test doesn't test much. If you enable all options (i.e disabled = false for all 3) it still passes, because the first option is selected and the key-based navigation skips it. It's got nothing to do with the new disabled
property. What do you think about making sure that the key-based navigation skips disabled options and test it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great idea! I'll work on a commit to push that behavior change up as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pushed up the requested behavior as part of my last commit. It refactors this unit test to check "keydown" events, and finally one to check "keyup" events as well.
test/live_select_test.exs
Outdated
type(live, "ABC") | ||
assert_options(live, ["A", "B", "C"]) | ||
# The maps are sorted on their key value pairings on the showcase page | ||
# before being sent to the LiveSelct component. This results in the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
Sets `normalize_values` to default `:disabled` to `false` if it wasn't explicitly set on an Option. This way each unit test doesn't need to explicitly set disabled. Adds new unit tests that exercises the disabled options in Single, Tags and Quick Tag modes. - Can disable by passing in options as a tuple or map. - Disabled options can't be selected - In tag mode, they stay disabled even after going under the max selection amount
Add documentation for how disabling options works, and how it can be used for tuples, maps and keywords.
2c16488
to
7bda955
Compare
navigate(live, 2, :down, []) | ||
|
||
# Navigate back to A by going 1 up as we skip B because it's disabled. | ||
# Then select the item we're on. It should be "A" | ||
navigate(live, 1, :up, []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the key up test, I wasn't sure if there was a better way, aside from navigating down, and then back up.
I kept it as a separate unit test rather than rolling it up into one so that we have one unit test proving "keydown" works as expected. Then this unit test proves key up works by building upon that.
Keybased navigation of Options now skips disabled options
8c778e7
to
50bcbfa
Compare
Hi @ZRothy1 , looks great 🥳 Thanks for the excellent work! 🚀 |
Updates LiveSelect to have the ability to disable an option in
:single
,:tags
and:quick_tags
mode. This way an option can not be selected like a disabled option in a regularselect
tag.Clicking a disabled option will not close the options dropdown, nor select the option. This has a minor behavior change to
:tags
mode. Before clicking an option after reaching the max selectable items would close the options dropdown, but leave the input focused.I believe this behavior change brings it closer to how clicking a disabled option would act with a
select
tag. Though if requested I can drop this change. It can also be limited if requested to only change the behavior for:single
mode and / or:tag
mode before the max items are selected.Finally I included one commit to add the
unavailable_option_class
as an attribute. It seems it was missing before, so setting it on a LiveSelect would emit a warning.