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

remove dependency on phoenix_html_helpers #299

Open
woylie opened this issue Dec 19, 2023 · 10 comments
Open

remove dependency on phoenix_html_helpers #299

woylie opened this issue Dec 19, 2023 · 10 comments
Assignees
Labels

Comments

@woylie
Copy link
Owner

woylie commented Dec 19, 2023

#298 updated phoenix_html to 4.0 and updated the calls to functions that were moved to phoenix_html_helpers. In the long run, the dependency on those functions should be removed completely.

@woylie woylie added the chore label Dec 19, 2023
@woylie woylie self-assigned this Dec 20, 2023
@linusdm
Copy link

linusdm commented Jan 3, 2024

In the documentation there is still mentioning of Phoenix.HTML.Tag (for setting the no_results_content option when rendering tables) and Phoenix.HTML.Form.inputs_for (when customising the filter form).

The first one might be replaced with a regular heex render, like this:

defmodule FlopHelpers do
  use MyAppWeb, :html

  def table_opts do
    assigns = %{}

    [
      no_results_content: ~H"""
      <p>no results found</p>
      """
    ]
  end
end

For the second use, I don't have a better alternative than using PhoenixHTMLHelpers.Form.inputs_for right now. But could probably be improved with regular components too.

@woylie
Copy link
Owner Author

woylie commented Jan 3, 2024

Replacing inputs_for with the component version is not possible, since the Phoenix.HTML.FormData implementation for Flop.Meta relies on the fields option being passed, but Phoenix.Component.inputs_for/1 doesn't allow to pass additional options at the moment. I'm not sure how to go about it.

@woylie
Copy link
Owner Author

woylie commented Jan 3, 2024

It would probably make sense to add an attr :opts, :list, default: [] to Phoenix.Component.inputs_for/1. The documentation for Phoenix.HTML.FormData.to_form/2 states:

The options have their meaning defined by the underlying implementation but all shared options below are expected to be implemented. All remaining options must be stored in the returned struct.

Since Phoenix.Components.inputs_for/1 uses the to_form function, it should be able to pass any options that the protocol implementation requires.

@linusdm
Copy link

linusdm commented Jan 4, 2024

That's a great insight. Thanks for taking the initiative upstream! That will allow a cleaner interface, more idiomatic in line with the new Phoenix Components. Very nice!

@jelkand
Copy link

jelkand commented Feb 20, 2024

Hello, I've been playing around with the use of Phoenix.Components.inputs_for/1 in order to style some of my own filter fields using live components, and I get a lot of console errors in the browser about duplicated IDs (screenshot below). My best guess is that something about the offset option is not being respected when provided via Phoenix.Components.inputs_for/1. It seems to work fine with Phoenix.HTMLHelpers.Form.inputs_for/4. Unfortunately I'm not able to spot any differences in implementation that would cause that.

image

@woylie
Copy link
Owner Author

woylie commented Feb 20, 2024

Yes, some flop_phoenix tests start failing when I switch to the inputs_for component because the IDs aren't as expected. I haven't found the issue yet.

@jelkand
Copy link

jelkand commented Mar 23, 2024

I have been looking into this further and it looks like the issue stems with these changes: phoenixframework/phoenix_live_view@1e169b8

This adds the concept of a _persistent_id to inputs_forand in doing so, it reassigns the id the Flop form set using the offset.

I believe the way to address this is to set the _persistent_id param in the HTML Form so that the .inputs_for component can pick it up and use it to generate proper IDs. That said I'm not too experienced with LiveView and I'm not sure if that is the way folks intended the _persistent_id to be used.

I'm happy to put up a PR to demonstrate, even if it ends up not being the right solution. Hopefully this can help your investigation as well.

@woylie
Copy link
Owner Author

woylie commented Mar 25, 2024

@jelkand That is very helpful, thanks for looking into this! I didn't have a closer look yet, but I'd like to understand better what the purpose of the _persistent_id, and whether this is something that could or should be solved on the LiveView side.

@jelkand
Copy link

jelkand commented Mar 25, 2024

I agree, and adding a _persistent_id to the params of the flop_phoenix form makes for a far-reaching change, especially considering that params is used internally to handle filters/query params etc. I believe the _persistent_id is used to support dynamically adding and removing form elements or forms while maintaining their ordering, but I haven't used it in anger.

@woylie woylie removed their assignment Oct 17, 2024
@woylie
Copy link
Owner Author

woylie commented Dec 2, 2024

This change was included in LiveView 1.0.0-rc.8: phoenixframework/phoenix_live_view@0d4f0ca. I didn't have a closer look at whether this changes anything yet.

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

No branches or pull requests

3 participants