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

Add sticky options #33

Merged
merged 4 commits into from Jul 20, 2023
Merged

Conversation

shamanime
Copy link
Contributor

Hi again!

This adds the ability to make options unremovable after they're selected.

In our app we share a form with 2 LiveViews and use LiveSelect to handle a nested association, in one of the pages we pre-set a default option that can't be removed but the user is still allowed to add/remove other options.

I've tried to follow your library standards and any feedback to improve my contribution is welcome.

Thanks!

@@ -615,7 +618,7 @@ defmodule LiveSelect.Component do
xmlns="http://www.w3.org/2000/svg"
viewBox="0 0 20 20"
fill="currentColor"
class="w-5 h-5 @class"
class={["w-5 h-5", @class]}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes a small bug which prevented the @class interpolation.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

Copy link
Owner

@maxmarcon maxmarcon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's brilliant! Thank you so much for this contributions. There's some stuff that can be removed/simplified, I left specific comments.

When we merge this one, I will release a new version

lib/live_select.ex Show resolved Hide resolved

option when is_binary(option) or is_atom(option) or is_number(option) ->
{:ok, %{label: option, value: option}}
{:ok, %{label: option, value: option, sticky: false}}

Copy link
Owner

@maxmarcon maxmarcon Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not change anything in normalize/1: there's no need to set a default value for the sticky flag, just like there is no need to set a default value for the tag_label key. If it's not there, then the option isn't sticky :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the tuple with 3 options to allow it to be sticky with tuples as well, if you think it's not needed we can remove.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah let's remove that as well and keep it simple and consistent with the tag_label option: if you want to use sticky or tag_label, then you have to pass a list of maps

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@@ -19,7 +19,7 @@
<% else %>
<%= render_slot(@tag, option) %>
<% end %>
<button type="button" data-idx={idx}>
<button :if={!option[:sticky]} type="button" data-idx={idx}>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is all we need to make it work!


assert_option_removeable(live, 2)
end

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love this!

test/live_select_tags_test.exs Outdated Show resolved Hide resolved
test/support/helpers.ex Outdated Show resolved Hide resolved
test/support/helpers.ex Show resolved Hide resolved
test/live_select_tags_test.exs Outdated Show resolved Hide resolved
test/live_select_tags_test.exs Outdated Show resolved Hide resolved
@shamanime
Copy link
Contributor Author

Thanks for taking the time to review. I've implemented your suggestions.

@shamanime shamanime requested a review from maxmarcon July 20, 2023 14:08
@maxmarcon maxmarcon merged commit cf83e9d into maxmarcon:main Jul 20, 2023
2 checks passed
@shamanime shamanime deleted the shamanime-sticky-options branch July 20, 2023 16:23
@maxmarcon
Copy link
Owner

looks great! Thanks again. I will release a new version tomorrow

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

Successfully merging this pull request may close these issues.

None yet

2 participants