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

Add phx-custom-events to allow custom events to be handled by LiveView #3438

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

superchris
Copy link

This PR adds support for a phx-custom-events attribute which will cause a private hook to be added to listen for Custom Events.

The hook will use the value for this attribute which should contain either a single or comma-delimited list of custom event names, which will be pushed to LV. The detail property of the custom event, merged with the element dataset, will be used as the payload of the event sent to LV.

Alternatively, if the phx-custom-events attribute is present to trigger this hook, the hook will look for any attributes which begin with phx-custom-event- and allow the user to rename the event e. g. phx-custom-event-foo="bar"

This code originated in the phoenix-custom-event-hook project and has been used on several projects.

@SteffenDE
Copy link
Collaborator

Just as a heads up: please don’t include the built assets (in priv) in your PR. I’ll have a closer look in the next days :)

@SteffenDE
Copy link
Collaborator

I very much appreciate the inclusion of an e2e test! I briefly skimmed over the code and it looks quite reasonable. That being said, I still don't really like the use of a magic separator for specifying multiple events (see also my initial comment here https://elixirforum.com/t/support-handling-customevents-in-liveview/64896/6). I don't have a clearly better solution though.

@chrismccord had some thoughts on this as well iirc; one important thing was the handling on non-serializable data in the event detail.

@superchris superchris marked this pull request as ready for review January 12, 2025 17:48
@superchris
Copy link
Author

@chrismccord love to hear your thoughts on this. I'm giving a talk on Custom Elements and LV and CodeBeam it would be great to have it merged by then :)

@superchris
Copy link
Author

@SteffenDE or @chrismccord presuming this approach seems reasonable, is there anything else I can do to help get this across the finish line?

@josevalim
Copy link
Member

@superchris Chris has just come back from holidays, so I assume he is putting his work/life back on track. This looks good to me but, since I don't have all context, we should wait for his feedback anyway. Thank you <3

@superchris
Copy link
Author

Sounds good, not trying to be a pest just didn't want this to get lost :)

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.

3 participants