Invalid plug layout option

[scorsi]

Hey,

I figure out that my torch pages where extending from my root layout. Doing plug :put_root_layout, {WerewolfWeb.LayoutView, :torch} instead of plug :put_layout, {WerewolfWeb.LayoutView, :torch} avoid having the main layout in addition to the torch's one.

It would be good to show how to do a better plug layout like here in the documentation. ;)

Thanks,

[cpjolicoeur]

I'm not sure the issue you are having here?

Did you somewhere else already manually put a root layout in place? The Torch controller has no root_layout set by default in the generated template, only the single layout used.

Did you manually add a root layout through your pipeline? If so, that really isn't something wrong with the Torch configs, as it's a manual customization you've made to your app as a whole and would need to be overridden as you did here as well.

[scorsi]

What's wrong is that the Torch layout is declaring a new html and body tags and if it is intended to be used within another root layout, then you should modify that to be HTML compliant.

In addition, the Torch layout when embedded in the Phoenix root layout is bugged, the nav bar items are reversed, certainly due to CSS issues.

I think, that the Torch layout is intended to be used as a root layout not an embedded layout. The default root layout is declared in the browser pipeline in the router file when creating a Phoenix project (at least since 1.5.0).

I'm so suggesting to change plug :put_layout, {WerewolfWeb.LayoutView, "torch.html"} into plug :put_root_layout, {WerewolfWeb.LayoutView, :torch}. And maybe directly in a new pipeline for Torch controllers.

[cpjolicoeur]

Sorry, maybe there was some communication issues with my last reply.

The Torch layout does included a fully compliant HTML DOM layout because it is intended to be used as a standalone layout, not nested within other layouts.

If you have customized your application to use another root layout, and not made the same corresponding changes to the generated Torch controller, then yes, you would see issues. But the defaults dont work this way unless you have already manually edited your application to use a global root layout by default.

The default root layout is declared in the browser pipeline in the router file when creating a Phoenix project (at least since 1.5.0).

I will take a look at the latest Phoenix default changes for 1.5 to see if there are changes to the default generators that we overlooked when updating for the 1.5 release. Our test suite for Torch includes a default Phoenix 1.5 app for integration testing, and the default for Phoenix 1.5.x (at the time of creating our tests) did not include any root_layout directives in the router file or anywhere else. A default/unmodified phx 1.5 application generated does not include any root_layout directives in the pipeline :browser macro, unless this has been added in more recent release of the 1.5.x codebase than was available when we created our tests initially the week of the first 1.5 release.

Could you generate a new default/new phx application and post it somewhere as a test case that we can take a look at?

And maybe directly in a new pipeline for Torch controllers.

By default Torch does not do anything with pipelines, as that is outside the scope of the project and would be up to the end-user to decide if/when/how they want to use Torch within a pipeline.

In addition, the Torch layout when embedded in the Phoenix root layout is bugged, the nav bar items are reversed, certainly due to CSS issues.

Lastly, I have no doubts that if/when the Torch layout would be embedded in another (of any kind) Phoenix root layout, the HTML and CSS would appear to be bugged, as this is not the intended use case.

[cpjolicoeur]

After looking briefly, it appears that Phoenix may bout the put_root_layout directive in place only if the phx app is generated with the live flag.

phoenixframework/phoenix@209ff93

Does your application with the issue, use live when you generated your app? If this is the app you are working with, I'd assume so based on what I see in your router file?

[cpjolicoeur]

After some initial investigation it appears that root layouts have only existed for about 3 months in the phoenix codebase, and where first available at all in the 1.14.16 release.

@scorsi Do you want to take a stab at making these changes for users installing torch on Phx 1.14.16 and above and submit a Pull Request?

[scorsi]

I was looking why we weren't aggreed about the default root layout option. It appears that it's only added when generating with live option (and between you and me, almost every new Phoenix project did add that option actually).

I should need to take a deeper look at Torch codebase to make this change where some of the project member could do it in a few minutes. But I could absolutely if you don't have the time for ;)

[cpjolicoeur]

It's up to you. Torch is a community project and we like to let everyone contribute to the project when they discover new issues or features of they want to. We can surely make the changes if you want, but I always like to just ask for the community participation if they want to.

Just let me know if you are going to try to open a PR, if not we can take care of it. Thanks

[scorsi]

Ok found where to do the changement.

I think always doing plug :put_root_layout, {WerewolfWeb.LayoutView, :torch} is a good idea even if there isn't any actual root layout. It ensures that there'll never have issue about this.
And if we do so, we should remove it from the Controller to put it in a pipeline. So, we shall add in the Readme and in the Mix task to add and use the Torch pipeline (as it is actually done for the Torch plug) for every Torch controllers. In addition, it sets the stage if people wants to ensure admin/auth for Torch controllers.

What do you think about my idea ? Do you validate it ?

users installing torch on Phx 1.14.16 and above

Do you mean 1.4.16 ?

[cpjolicoeur]

@scorsi Yes, sorry, I meant version 1.4.16 and above.

Torch would need to have the generator only add put_root_layout instead of put_layout for users running the generator on version 1.4.16 and above, as older versions do not even support it.

I'm not sure about making this change outside of the controller though. Putting this only in the Torch generated controllers ensures it is only used on Torch specific controllers and would override any other root layouts that may already be in place.

Currently, Torch does not make any pipelines or modify the applications router file in any way. Are you suggesting the Torch generators make a new dedicated Torch pipeline (e.g. pipeline :torch do...end)?

Currently the Torch application can choose to put the routes for a Torch controller anywhere they want.

[scorsi]

Here's what I'm suggesting :

pipeline :torch do
    plug :put_root_layout, {MyAppWeb.LayoutView, :torch}
end

scope "/admin", MyAppWeb, as: :admin do
        pipe_through [:browser, :torch]
    
        ...

        resources "/posts", PostController
end

Where put_root_layout appears only for users having Phoenix >= 1.4.16, instead the same code is written with put_layout for other Phoenix versions.

As I said, it will sets the stage for users wanting more (ensure users are authenticated and admin for example) and it doesn't require lot of modification and didn't breaks the browser pipeline event with different Phoenix versions.

[scorsi]

I think putting the layout apart from controllers is better to edit it if needed (if a have a dozens of controllers and I want to switch the layout to a new one).
Every code should avoid code duplications and this is clearly one. But I can admit it adds some complexity and requires better Phoenix understanding.
It all depends of you, I'll do as you suggests ;)

[cpjolicoeur]

What is the value add here though, vs how we currently do it directly in the controller?

In your suggested scenario, we create a new minimal pipeline that users also will need to be aware and maintain with their routing going forward, vs always explicitly setting the root layout in each Torch controller like we currently do?

I'm not sure the added value with the dedicated pipeline. Phoenix users who use Torch already can make and combine pipelines and other features in the router as desired, and these modifications really wouldn't affect that (or add any additional benefits). Keeping the put_root_layout in the Torch controllers specifically, however, would allow applications to use your pipeline scenario for the /admin scope to use a default root layout for any non-Torch controllers that may fall under admin, while still having the Torch controllers use the Torch specific layout as an override (by using put_root_layout in the Torch controller file where it currently lives).

Things like authentication, admin, response types, caching, cookies handling, other plugs, etc... are all outside the scope of what Torch provides and I'm hesitant to start having Torch expand it's reach and start being more declarative about where and how it's used if we don't have to.

[cpjolicoeur]

I think putting the layout apart from controllers is better to edit it if needed (if a have a dozens of controllers and I want to switch the layout to a new one).
Every code should avoid code duplications and this is clearly one. But I can admit it adds some complexity and requires better Phoenix understanding.
It all depends of you, I'll do as you suggests ;)

While there would be some duplication in the actual code, I don't think, in practice, there is duplicated work in editing the layouts as you suggest. If a user wants to modify the Torch layout markup, all they need to do is edit the torch layout file as all torch controllers use the same Torch layout file by default.

[scorsi]

I think I was unclear when trying to explain my idea ^^

What I thought is if a guy want to make it's own admin in his project and only want to edit the Torch layout to be embedded into its own Admin root layout separated from the other root layout. He will have to edit every already created and future controllers put_layout plug to match his needs (without forgetting to do it). Where with the pipeline he could edit every already created and future controllers put_layout plug since it'll be declared in the pipeline instead that in the controllers.
It's actually my case. I have to manually edit every Torch controller to remove that plug.
I can deeper explain with code if you want to.

But if you don't want Torch to add configuration to Phoenix files (except the plug which can be anywhere-else), I'll do as you ask since it's your project and so for people doing things like I do will have to make the changes in each controllers.
I stop forcing for this Torch pipeline from now. =)

[cpjolicoeur]

I see what you are describing now. I think the use-case you describe is pretty rare, even if it happens to be something you are specifically facing yourself. The "average" Torch user doesn't really fall into that level of customization. If the application is undergoing that level of specific customization, then I think the developer(s) of that application already understand that there will be some additional work involved since they are already deciding to make those deeper changes to begin with.

Meaning, if an app developer is already going to the length to further customize their own admin-layer root layout, then it's not unreasonable for them to expect to have to make a few other changes to the generated code along the way. They already know they are making a deeper/more extensive change and so the side effects involved will be larger (in this case having to update/edit N number of existing Torch controllers).

One potential option could be to add a pipeline option to the torch.install task that would give the user the option to opt-in to having a specific :torch pipeline created and added to the router when Torch is first installed. We would need to somehow carry the selected option of this flag though through so that the torch.gen.html tasks know whether or not to put the put_root_layout code into the controller or not. Perhaps this would be an option on config :torch, pipeline: true or something like that. Then all the torch specific mix tasks could reference them. Just thinking out loud here, haven't really run all this through my head yet completely.