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

Adds :filter_values option to conditionally keep or remove items from the selection #64

Closed
wants to merge 4 commits into from

Conversation

shamanime
Copy link
Contributor

This is based off #63 this is why it has both changes.

This allows running a function against selected values to keep/discard items.

This is the motivation behind this change:

We allow the user to "tag" other users when creating some records using LiveSelect.
There is a field that controls if the record is "sensitive".
When that field changes, if it changes to "sensitive", we must remove previously "tagged" users that can't see sensitive information.

@maxmarcon
Copy link
Owner

Hi and sorry for reacting so late.

In the spirit of keeping the API small, can't we achieve this by filtering the options available to the user and/or programmatically reset the selection using:

send_update(LiveSelect.Component, id: live_select_id, value: filtered_selection)

as I was suggesting in #63 ?

@shamanime
Copy link
Contributor Author

Hi and sorry for reacting so late.

In the spirit of keeping the API small, can't we achieve this by filtering the options available to the user and/or programmatically reset the selection using:

send_update(LiveSelect.Component, id: live_select_id, value: filtered_selection)

as I was suggesting in #63 ?

No worries.

That's a good suggestion. I didn't consider doing that because I've got a few scenarios with forms having multiple LiveSelects as the source of truth and I'm not keeping the selection state on the LiveView/Component anymore (we were doing that before and it was the main reason to start using LiveSelect), so when the feature was requested and we figured out the events/API that we wanted for the interactions, it felt that the right way was to delegate it to the library because it has such a nice API and makes the code much more cohesive.

The main benefit to me is to keep the LiveView code easy to reason about.
Right now we have a very clear flow: Anything happens on the LV -> New options/append/filter_selection [without knowing the current state] are sent to LiveSelect -> LV receives the new updated selection from LiveSelect and updates the from it. It works wonders.
By bringing the selection back to the LV we open the door to directly use it (guilty as charged), and end up with two places holding the same state that we may forget to keep in sync.

However I do understand you wanting to keep the API small and there's no hurt feelings if you think this is out of scope.

@maxmarcon
Copy link
Owner

maxmarcon commented Jun 9, 2024

Thanks for the explanation. I'm starting to see the value in this approach.

Instead of using 2 new options, one to pass a filter and the other to append to the selection, my suggestion would be to generalize the approach by passing an update_selection/1 function that receives the whole selection and returns the new one:

append case:

values_to_append = [1, 2, 3]

send_update(LiveSelect.Component, id: live_select_id, update_selection: fn selection -> selection ++ values_to_append end)

filter case:

send_update(LiveSelect.Component, id: live_select_id, update_selection: fn selection -> Enum.filter(selection, & &1 > 2)
end)

2 birds with a stone. And this general approach will hopefully accommodate future use cases.

What do you think?

EDIT: this would also make sense in single mode

@shamanime
Copy link
Contributor Author

I think that does make a lot of sense.

I've got a few busy weeks ahead but would love to give it a shot whenever I can if you don't mind.

Would you rather have me update this or open a new PR?

@maxmarcon
Copy link
Owner

I think it's better to close these 2 PRs and open a new one that references both.

I've got a few busy weeks ahead but would love to give it a shot whenever I can if you don't mind.

That sounds great! Thanks

@shamanime
Copy link
Contributor Author

Closed in favor of #68

@shamanime shamanime closed this Jun 9, 2024
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.

2 participants