search

mgwidmann / scrivener_html

HTML view helpers for Scrivener
MIT License
125 stars 208 forks source link

Add support for Phoenix.LiveView.live_link/2 #93

Closed mcrumm closed 4 years ago

mcrumm commented 5 years ago

Hi @mgwidmann, thanks for a great library!

When using Scrivener.HTML with Phoenix.LiveView, some further integration was required to get links generated via the live_link/2 helper.

This PR will check a custom :action to determine whether or not the action refers to a Phoenix.LiveView. If so, live_link/2 will be invoked. Without LiveView, or with a "regular" action, we continue to invoke Phoenix.HTML.Link.link/2.

A few notes:

1) LiveView requires Phoenix.HTML ~> 2.13.2, which is a pretty significant bump from the currently pinned version. 2) There are some regressions in the existing tests, unrelated to the live_link/2 changes. They appear to all be changes to the safe HTML output, not functional regressions. I fixed an easy one. :D 3) Currently no warning is emitted if you attempt to use a module that is not a LiveView as an action -- it just silently falls back to link/2. 4) An optimization should probably be added for common actions to avoid the expensive module checks. I'm thinking we can match on the Phoenix resource actions at least. 5) I need to write tests for the LiveView integration. With the addition of Floki, I was thinking I would test HTML, as opposed to the safe, output, as we really just need to do look for the addition of the data-phx-live-link attribute.

Thoughts?

speeddragon commented 5 years ago

I think there is already one open PR to support live_link/2 function.

mcrumm commented 5 years ago

@speeddragon @minibikini D'oh, I didn't even see the other PR! 🤦‍♂

That said, I'd love to not have to pass another override every time I want to use live_link, but there could be other advantages to making the link function generic. Futhermore, LiveView might not have settled down enough yet for this level of integration, but the idea of live pagination is really appealing.

mgwidmann commented 4 years ago

Closing in favor of #91