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: Makes Host Project Layouts Available via beacon_site macro #431

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

Conversation

ddink
Copy link

@ddink ddink commented Mar 7, 2024

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

These changes make it possible to define the root_layout and on_mount session options that are passed into the live_session in the beacon_site macro (via Beacon.Router.__options__/1).

Adding these changes allow the host project to use an existing layout to house Beacon's content.

Screenshot 2024-03-07 at 11 09 32 AM Screenshot 2024-03-07 at 11 04 31 AM

Then the only thing that has to be done to use these options is to pass the additional options into the beacon_site call as a keyword list:

 beacon_site "/my_site",
      site: :my_site,
      root_layout: {MyApp.Layouts, :app},
      on_mount: {MyApp.Init, :init_data}

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

This feature is a business need. It will benefit other users by giving them the ability to use a layout that's defined in their host project. It will also allow other users to leverage the on_mount session option on all Beacon pages.

Are there any drawbacks to this feature?

I don't see any drawbacks to this feature since it only configures the options that are already available for change
in Phoenix.LiveView.Router.live_session/3.

@leandrocp
Copy link
Contributor

Hey @ddink I appreciate the PR!

Are there any drawbacks to this feature?

Unfortunately there's a drawback that is caused by the fact that Beacon requires some functions in the root layout in order to work properly ⬇️

<%= render_meta_tags(assigns) %>
<%= render_schema(assigns) %>
<.live_title>
<%= render_page_title(assigns) %>
</.live_title>
<%= render_resource_links(assigns) %>
<link id="beacon-runtime-stylesheet" rel="stylesheet" href={asset_path(@conn, :css)} />
<script defer src={asset_path(@conn, :js)}>

If you don't provide those functions, Beacon rendering breaks or at least cause undesired behavior like not connection to LV, not rendering data that you'd expect it would render on pages, missing styles, and so on.

Ideally we want to find a solution to "merge" or "inherit" layouts instead of just overwriting it because it needs both Beacon and your app functionality. Does it make sense?

I'm still not sure what's the best approach but I hope this gives some light why I din't merge this one yet.

@ddink
Copy link
Author

ddink commented May 1, 2024

Hi @leandrocp yea I understand the dilemma here. It does make sense for Beacon to be focused on its own root layout. However, I would first consider whether or not all applications using Beacon will use the CMS universally.

Using Beacon’s necessary layout functions in our existing layout caused issues with the existing live views in our codebase. Beacon pages come with some overhead that can’t be left out (such as the __dynamic_page_id__ assigns). I suppose this means that every route in an application using Beacon must be a Beacon page, which makes Beacon monolithic in behavior. For us, porting over all of our existing routes, live view logic and templates to Beacon isn’t a good solution, because of the potential for non-engineers to break code or for engineers to lose version control over our live views and templates.

Additionally, our application has on_mount hooks that are needed on each page, because they provide data to the layout. Some of our hooks need to be able to read data from the phoenix live session, which seems like another obstacle.

The way we’re getting around it is by 1) overriding Beacon’s runtime.html.heex with our own duplicate layout for our CMS pages and 2) adding those functions to the duplicate via a module that imports those functions from BeaconWeb.Layouts. As long as there is a clear boundary between our non-Beacon pages and our Beacon pages, it works.

The thing for us is that we designed our application so that the non-CMS pages don’t need any CMS-able components (although, it would definitely be worth it to have that functionality available to non-CMS pages at some point). I think the difficulty for Beacon is how do you make dynamic pages available to the CMS for editing without eliminating the practicality of just having dynamic pages that don’t depend on the CMS?

I can already access the necessary data to build our dynamic pages from within Beacon. But doing so is also impractical. Keeping those live view templates in our codebase makes it easier for us to edit, version control, and has the bonus benefit of preventing non-engineers from mistakenly breaking anything. We get more flexibility from not using Beacon as a monolith.

In other words, we don’t need to override Beacon’s runtime.html.heex layout, but we are doing so because working with live views from within the codebase offers more benefit than building our dynamic pages in the CMS. So for me the real problem is that it isn’t always practical to build dynamic pages from within the CMS.

In our case at least, it would be more practical for our codebase to have access to Beacon’s components outside of the CMS—perhaps via a function call that doesn’t require Beacon’s overhead (something like Beacon.Content.Component.display(”App Header”) for example). However, I do believe that would require an additional PR with a wider scope than this one.

I’m not trying to disagree with Beacon’s approach here, so much as making the argument that there are use cases for Beacon that don’t include its use as a monolith. Our application is an umbrella where the web interface application is just one of several child apps. Having the option to use Beacon without it being the only way that we make changes to our web interface is our ideal use case. Being able to incorporate Beacon’s components into our dynamic pages would be a step further in the direction of our ideal use case.

@ddink ddink force-pushed the add_live_session_options branch from d016a18 to c443ff9 Compare May 10, 2024 20:06
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.

None yet

2 participants