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

Feature: add site-wide handle_info callbacks #578

Merged

Conversation

ddink
Copy link
Contributor

@ddink ddink commented Aug 23, 2024

What will it allow you to do that you can't do today?

These changes will make it possible to add site-wide handle_info callbacks that will be applied to each Beacon page. There is also a complementary pull-request to beacon_live_admin that adds support for administering the CRUD of these site-wide callbacks.

Why do you need this feature and how will it benefit other users?

This feature will give users the ability to handle messages sent by Kernel.send/2 . Commonly, this can be used to do things like put flash messages, navigate to another LiveView, work with streams, etc. in response to messages sent by the application.

Are there any drawbacks to this feature?

  • This feature is not designed to create page-specific handle_info callbacks—just site-wide callbacks that are useable on all of a site’s pages. I’m happy to add a page-specific handle_info callbacks feature at a later point, though.

  • Changes to handle_info callbacks through the admin require a recompile before they are useable. They are not immediately useable upon a refresh of a Beacon page the way that an update to a Beacon page is immediately viewable.

@ddink ddink force-pushed the site_wide_handle_info_callbacks_feature branch 3 times, most recently from 70d9489 to ff037d9 Compare August 23, 2024 20:47
@ddink ddink force-pushed the site_wide_handle_info_callbacks_feature branch from ff037d9 to 92a4a38 Compare August 28, 2024 20:34
@leandrocp leandrocp self-requested a review September 9, 2024 14:14
Copy link
Contributor

@leandrocp leandrocp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ddink great work! Sorry for the delay, things piled up with ElixirConf. In overall it's looking good, just left a few comments that we need to address before merging it. Thanks!

lib/beacon/loader/page.ex Outdated Show resolved Hide resolved
lib/beacon/loader/page.ex Show resolved Hide resolved
lib/beacon/migrations/v001.ex Outdated Show resolved Hide resolved
lib/beacon/content.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@leandrocp leandrocp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ddink one last small change and then we're done with this PR. I'm gonna check the live_admin PR now. Thanks!

lib/beacon/content.ex Outdated Show resolved Hide resolved
lib/beacon/content.ex Outdated Show resolved Hide resolved
Copy link
Contributor

@leandrocp leandrocp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ddink that looks good, great contribution! I'll merge into main and release right after v0.1 since that version is frozen for new features, but that should happen soon. Thanks!

@leandrocp leandrocp merged commit a46e29f into BeaconCMS:main Sep 20, 2024
2 of 3 checks passed
@leandrocp
Copy link
Contributor

Thanks @ddink !

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.

2 participants