live_select icon indicating copy to clipboard operation
live_select copied to clipboard

chore: add `required` option to LiveSelect

Open itamm15 opened this issue 10 months ago • 20 comments

Is there any obstacle that LiveSelect currently doesn't support a required attr? If not, then I can work on adding that :)

itamm15 avatar Mar 12 '25 08:03 itamm15

I actually came here to ask for the same thing. The omission of this basic HTML functionality is somewhat bewildering.

egeersoz avatar Mar 12 '25 12:03 egeersoz

@itamm15 required is an attribute for front-end HTML validations controlled by the browser. LiveView forms work with validations that run on the backend. That's the reason why I never thought of adding it.

Can you please elaborate a bit on when/how you would use it and why do you believe it's necessary?

If we decided to add it, an immediate problem that I can see is that in tags mode the text input element will be empty even if something is selected, and the front-end validation will fail, even it really shouldn't.

maxmarcon avatar Mar 12 '25 12:03 maxmarcon

Hi @maxmarcon, thanks! The reason is simple - what we have to do now if we want to require the select to not be empty is;

  1. Assign a variable on mount level, is_form_valid: false,
  2. You the variable in the button, <button disabled={!is_form_valid} ...>,
  3. Then, add the validation via phx-chang="validate-form",
  4. Check on the server level, whether the select is not empty - if is, then don't change the is_form_valid state, else is_form_valid = true.
  5. The button is enabled, and the user can proceed with some action.

This is our current workaround for the required option. This whole workflow could be simply substituted by the required option.

Regarding tags - sorry, I am not that aware of this functionality, but maybe we could simply drop the required option for this mode? (in other words, forbid required option to be assigned to the LiveSelect with tags mode)

itamm15 avatar Mar 12 '25 13:03 itamm15

Hi and thanks. Ok so you're not using Ecto schemas and changesets to validate your form? Because if so I believe all you'd have to do would be to use validate_required/3 and bind the disabled attribute of your button to the valid status of the form.

maxmarcon avatar Mar 12 '25 13:03 maxmarcon

Regarding the changsets for form - generally, yes, we do use. But this one is a bit more complicated, I would say, because we use interceptors (designed by our RAP system), and before actually executing the function triggered from the click on the form, the interceptor is being fired (and the required field is needed for the interceptor logic, not the changeset itself). Scenario without required: We have a create event for a resource and also defined interceptors to check the permissions before executing the create event.

  1. User goes to the form and doesn't fill the select (that's meant to be required, so we do normally require/validate it as described in my first comment),
  2. The user clicks the create button, and interceptors are executed. Hence, boom 💥 happens 'cause the input hasn't been filled!

Hope that makes sense 😄

itamm15 avatar Mar 12 '25 13:03 itamm15

I don't know what you mean by interceptor or by RAP. But if I understood you correctly, you have some front-end third-party code that expects a required attribute in the input field? It's the only case where I can see that adding the attribute might be justified, because in all other cases you can and should do everything on the server

maxmarcon avatar Mar 12 '25 17:03 maxmarcon

Apologies for not being clear!

  • RAP - roles and permissions system
  • interceptors - https://hexdocs.pm/phoenix_live_view/Phoenix.LiveView.html#attach_hook/4

Sample LiveView (pseudocode)

def LiveView do
 use LiveView, :live_view

 on_mount(LiveView.Interceptors, {:create, :can_create})

 def render(assigngs) do
 ~H"""
  <form phx-change="create">
   <.live_select id="some_attr_that_should_be_required" />
  </form>
 """
 end

 def mount(_params, _session, socket), do: {:ok, socket}

 def handle_info(:create, params, socket) do
  # creation logic
 end 
end

def LiveView.Interceptors do
 def mount(interceptors, params, socket) do
  value_from_live_select = params["some_attr_that_should_be_required"]
  attach_hook(interceptors, ..., fn -> end)
 end 
end 

As the value from LiveSelect is used in the interceptors (because it is required attr in the interceptors to check permissions), we are not able to validate it in the create/2 event. I hope that makes it clearer!

itamm15 avatar Mar 12 '25 19:03 itamm15

ok thanks, it's getting a bit clearer, still a lot of questions. I'll pick 2:

  1. why using hooks to verify if a field of a form is set instead of doing it in the submit or change events?
  2. which stage of the LiveView lifecycle are you tapping into and how are the forms parameters being accessed by the hook? (Your code doesn't show that)

maxmarcon avatar Mar 12 '25 19:03 maxmarcon

  1. So, we are checking if the field was settled in the form - from this perspective, we require it to be settled. It is just needed for further checks (e.g. db queries). (and as stated previously, now we mimicked the behaviour of the required option for LiveSelect with the change event, but the required option would just be simpler and definitely sufficient for us, as we just require this field to be settled, not actually validated),
  2. So, this is done automatically by/within the attach_hook/4! 😄

Image

https://www.youtube.com/watch?v=8UQLPQDqkMQ

itamm15 avatar Mar 12 '25 20:03 itamm15

sorry, but you didn't really answer my questions. I know how attach_hook works, that's why I asked you which stage you're attaching to (handle_params, handle_event etc. ?) and how the code in your hook is accessing the parameters of the form.

Regardless of the way in which you access the form, I don't understand why you need required attribute at the HTML level. You have the form with the parameters I presume? Then you can just check whether the field you need is set 🤷 . Please tell me what I'm missing and maybe send me some code with a complete example of what you're trying to do. Thanks.

maxmarcon avatar Mar 12 '25 20:03 maxmarcon

Sorry for all of the confusion, @maxmarcon! So, my main argument for the required option would be just the ease of usage. As you correctly pointed, we can achieve the same with the phx-change and then validation on the server level. But that's just a few more lines of code and an additional call to the server in exchange for 1 line and a client check.

itamm15 avatar Mar 12 '25 21:03 itamm15

The required attribute is helpful when we have a form with a Submit button. If the user clicks the Submit button without entering anything in the input, the browser will automatically prompt the user to fill in the field. It's as simple as that.

olafbado avatar Mar 13 '25 07:03 olafbado

Correct. It's a best practice when it comes to HTML forms to rely on built-in HTML validations as much as possible. The required attribute in particular is really nice because not only does it let you enforce the validation with zero code, but also saves you bandwidth.

egeersoz avatar Mar 13 '25 07:03 egeersoz

@olafbado see my comment above. You can do all this in LiveView

@egeersoz I don't think you're saving any bandwidth. Remember, every time the user changes something in the form in LiveView, this is a round-trip to the server. You can always bind everything in your HTML, including whether a submit button is disabled, to the state of the changeset that is backing your form. This is the basic idea behind LiveView forms, and how errors are usually rendered.

Anyway, you all seem to be really eager to have this, and I just saw that Phoenix core components have required in the list of global attributes allowed in the input fields. Maybe we can create a MR that lets the global attributes through (including required) and sets them in the text input field.

Would this be ok?

maxmarcon avatar Mar 13 '25 09:03 maxmarcon

Hi @maxmarcon! What you mean is that all of the attrs provided by Phoenix will be supported by LiveSelect, right?

itamm15 avatar Mar 13 '25 10:03 itamm15

Remember, every time the user changes something in the form in LiveView, this is a round-trip to the server.

I don't believe this is accurate, and I verified it by looking at the traffic in my browser's console. Server roundtrips on user changes happen only if the form has a phx-change handler, which is optional. The most common scenario is to simply use a phx-submit, where all changes are local and the data is sent (and validated) only when the form is submitted.

Some changes necessitate "real-time" validation using phx-change. Many others don't, and in those scenarios forcing round-trips is a waste of bandwidth and server processing power.

egeersoz avatar Mar 13 '25 10:03 egeersoz

I don't believe this is accurate, and I verified it by looking at the traffic in my browser's console. Server roundtrips on user changes happen only if the form has a phx-change handler, which is optional. The most common scenario is to simply use a phx-submit, where all changes are local and the data is sent (and validated) only when the form is submitted.

Yes, I was assuming one is using phx-change and phx-submit because this is the basic idea behind LiveView: all validations and all the logic reside on the server. I highly doubt that the most common scenario is just using phx-submit as you write. Just take a look at the first paragraph of the "Form Bindings" section in the LiveView docs:

To handle form changes and submissions, use the phx-change and phx-submit events. In general, it is preferred to handle input changes at the form level, where all form fields are passed to the LiveView's callback given any single input change

Pretty unambiguous statement I would say.

To me, mixing front-end and back-end validations and logic in LV (unless strictly necessary) is mising the point of the framework. Moreover, if you're worried about this level of bandwidth saving (passing small events over a websocket) then I think you shouldn't use LV but a front-end framework. Anyway, just my opinions and I'm not gonna tell you how you have to design your application 😉

maxmarcon avatar Mar 14 '25 08:03 maxmarcon

@itamm15

Hi @maxmarcon! What you mean is that all of the attrs provided by Phoenix will be supported by LiveSelect, right?

The idea would be to add an attribute to the form component along these lines (code taken from the Phoenix core components, just an example):

  attr :rest, :global,
    include: ~w(accept autocomplete capture cols disabled form list max maxlength min minlength
                multiple pattern placeholder readonly required rows size step)

where you add a catchall rest attribute for the global HTML attributes, and you render it like this in the text input:

<input .... {@rest} />

It's important we take special care that @rest doesn't allow overriding other attributes set internally by the component, most importantly the class attribute. If you want to take a stab at it, you're very welcome.

maxmarcon avatar Mar 14 '25 08:03 maxmarcon

Hey @maxmarcon! Okay, that's what I thought initially! Yup, I would like to take care of that. Are you sure we need all of these attrs, though? 🤔 (if you want to, we can discuss this matter in the MR)

itamm15 avatar Mar 14 '25 09:03 itamm15

@itamm15 No, my gut feeling is that we should start small and only allow the attributes we need (required in this case) but I don't know how you can forbid the HTML class attribute in the :global component attribute type (haven't looked at this stuff in a long time)

I guess considering the different options is part of the job of working on the MR. Yes, further discussion is best handled there. Thanks

maxmarcon avatar Mar 14 '25 09:03 maxmarcon