Move phx-feedback-for to the parent element of a form element

[sezaru]

Hi,

Currently, all the form fields have the phx-feedback-for attribute inside the element itself. For example, for the text input, we have:

def text_input(assigns) do
    assigns = assign_defaults(assigns, text_input_classes(field_has_errors?(assigns)))

    ~H"""
    <%= Form.text_input(
      @form,
      @field,
      [class: @classes, phx_feedback_for: Form.input_name(@form, @field)] ++ Map.to_list(@rest)
    ) %>
    """
  end

The problem with that is that if you add a phx-update="ignore" to that element (you would do that for inputs that you want to use a hook that will change the element value, for example), now phx-feedback-for will never be added to that element, but the has-error class will, meaning that that element will show an error after a form validation even if that element was not touched yet.

To fix this, we need to move the phx-feedback-for outside of the form element and change the CSS a little bit.

Just as an example/suggestion, for the text_input, we can do this:

  def text_input(assigns) do
    assigns = assign_defaults(assigns, text_input_classes(field_has_errors?(assigns)))

    ~H"""
    <div phx-feedback-for={Form.input_name(@form, @field)}>
      <%= Form.text_input(
        @form,
        @field,
        [class: @classes] ++ Map.to_list(@rest)
      ) %>
    </div>
    """
  end

And change the CSS to this:

div:not(.phx-no-feedback) > input.has-error {
  @apply !border-red-500 focus:!border-red-500 !text-red-900 !placeholder-red-700 !bg-red-50 dark:!text-red-100 dark:!placeholder-red-300 dark:!bg-red-900 focus:!ring-red-500;
}

That way even if we add the phx-update="change" attribute, the element will still show errors correctly.

[sezaru]

Thinking more about this, I would also argue that we should also move the has-error field to the div instead of the input.

If we do that, we would also solve #159

He is an example of that to the select input:

  def select(assigns) do
    error_class = if field_has_errors?(assigns), do: "has-error", else: ""

    assigns =
      assigns |> assign_defaults("pc-select") |> assign(:error_class, error_class)

    ~H"""
    <div phx-feedback-for={Form.input_name(@form, @field)} class={@error_class}>
      <%= Form.select(
        @form,
        @field,
        @options,
        [class: @classes] ++ Map.to_list(@rest)
      ) %>
    </div>
    """
  end

And change the CSS to this:

div.has-error:not(.phx-no-feedback) > select {
  @apply !border-red-500 focus:!border-red-500 !text-red-900 !placeholder-red-700 !bg-red-50 dark:!text-red-100 dark:!placeholder-red-300 dark:!bg-red-900 focus:!ring-red-500;
}

[sezaru]

Btw, if you are OK with the change I can create a PR with it for all form components

[mplatts]

One issue could be you now are returning a div instead of an input, which may be unexpected or could create issues with styling. You might want to add styling to that div for layout purposes.

Maybe a halfway point could be adding your new classes so people can wrap their input with <div phx-feedback-for={Form.input_name(@form, @field)}> and it'll still work with the phx-update="ignore"? That's what I do with <.form_field>

[sezaru]

Not sure if I understand your suggestion, can you clarify it? Do you mean manually adding the div outside the component? If so, that would work, but I believe that the user would also need to remove/overload the current CSS from petal components and add a new one like the one I showed above.

In that case, it seems to me that the user is better off just copying the full form.ex file from Petal Components and changing it instead of trying to work around it.

I don't see what is the problem with actually returning the div instead of the input as the root tbh, as long as that div doesn't have any styling on it, it should just be "invisible" to the end user no?

Also, even core_components does that, seems like that is the "correct" way of handling these cases since inputs have a lot of corner cases (like the select input discussed in #159).

Edit: Also, if there is a need to allow adding classes/styling that div, then we can just add a new optional attr to the components to add classes to it no?

[mplatts]

Yeah, I meant manually adding a div - perhaps in a new custom component that utilises the <.text_input> component or whichever one you were using. eg

eg

def my_new_field do
  ~H"""
  <div phx-feedback-for={@field}>
    <.form_label form={@form} field={@field} />
    <.text_input phx-hook="xxx" field={@field} form={@form} field={@field} />
    <.form_field_error form={@form} field={@field} />
  </div>
  """
end

The core_components input function is more like our form_field function - it contains the label, input and error components.

Can I ask what hook you are implementing?

[mplatts]

Done in v1.1.0 with the new <.field> component. 791276f

Docs coming soon