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

[Turbo] Include minimal layout for Turbo Frames #2318

Open
wants to merge 10 commits into
base: 2.x
Choose a base branch
from

Conversation

seb-jean
Copy link
Contributor

@seb-jean seb-jean commented Nov 1, 2024

Q A
Bug fix? no
New feature? yes
License MIT

Since Turbo does not need the content outside of the frame, reducing the amount that is rendered can be a useful optimisation.

This PR is the Symfony version of Turbo Rails: hotwired/turbo-rails#428.

To use it, you must do the following:

{% extends '@Turbo/frame.html.twig' %}

{% block body %}
    <turbo-frame id="frame_id">
        Content of the Turbo Frame
    </turbo-frame>
{% endblock %}

{# renders as: #}
<!DOCTYPE html>
<html>
    <head>
    </head>
    <body>
        <turbo-frame id="frame_id">
            Content of the Turbo Frame
        </turbo-frame>
    </body>
</html>

@carsonbot carsonbot added Feature New Feature Status: Needs Review Needs to be reviewed labels Nov 1, 2024
@seb-jean seb-jean changed the title Include minimal layout for Turbo Frames [Turbo] Include minimal layout for Turbo Frames Nov 1, 2024
@smnandre
Copy link
Member

smnandre commented Nov 1, 2024

What would be the use case ?

@smnandre
Copy link
Member

smnandre commented Nov 2, 2024

As i understand the discussions from the issue you linked, the need really is only for people wanting to set specific head metadata (title, canonical, turbo config)... in which case i'd say this template will be really specific per app

Also, Symfony initially come with a template very similar

<!DOCTYPE html>
<html>
    <head>
        <meta charset="UTF-8">
        <title>{% block title %}Welcome!{% endblock %}</title>
        <link rel="icon" href="data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 128 128%22><text y=%221.2em%22 font-size=%2296%22>⚫️</text><text y=%221.3em%22 x=%220.2em%22 font-size=%2276%22 fill=%22%23fff%22>sf</text></svg>">
        {% block stylesheets %}
        {% endblock %}

        {% block javascripts %}
            {% block importmap %}{{ importmap('app') }}{% endblock %}
        {% endblock %}
    </head>
    <body>
        {% block body %}{% endblock %}
    </body>
</html>

So i'm not sure we should add another "empty/base" template 🤷

To only render a frame, you can wrap it in a block and use the AbstractController::renderBlock, or (possible) i'm missing something ?

@seb-jean
Copy link
Contributor Author

seb-jean commented Nov 3, 2024

As i understand the discussions from the issue you linked, the need really is only for people wanting to set specific head metadata (title, canonical, turbo config)...

Yes, that's exactly it.

@smnandre
Copy link
Member

smnandre commented Nov 3, 2024

But this would be super specific, with probably common code with their codebase, right ? I'm just wondering if this can be really used ?

Maybe some documentation to explain why an "almost empty" layout can be used seems to me a good idea, no ?

@smnandre smnandre added Status: Waiting Feedback Needs feedback from the author Turbo and removed Status: Needs Review Needs to be reviewed labels Nov 3, 2024
@seb-jean
Copy link
Contributor Author

seb-jean commented Nov 6, 2024

But this would be super specific, with probably common code with their codebase, right ? I'm just wondering if this can be really used ?

Maybe some documentation to explain why an "almost empty" layout can be used seems to me a good idea, no ?

Done.

@carsonbot carsonbot added the Status: Reviewed Has been reviewed by a maintainer label Nov 7, 2024
@smnandre
Copy link
Member

smnandre commented Nov 7, 2024

What i meant was... i'm not sure if a documentation would not be enough?

The template now makes 6 lines, this could be on documentation.

Maintaining an almost empty file does not seem a good idea to me.

Especially as the tag needs a lang attribute, and we should not call app.currentLocale ourselves :|

Can we just add documentation, remove the added file, and wait to see if this really is a need for users (even then, i believe a file in the bundle recipe would be a better idea)

@smnandre smnandre added docs Improvements or additions to documentation and removed Status: Reviewed Has been reviewed by a maintainer Feature New Feature labels Nov 7, 2024

The minimal layout for Turbo Frames was added in Turbo 2.22.

Since Turbo does not need the content outside of the frame, reducing the amount that is rendered can be a useful optimisation. However, this optimisation prevents responses from specifying ``head`` content as well. There are cases where it would be useful for Turbo to have access to items specified in ``head``. To specify the heads, you must then use a minimal layout for frame, rather than no layout. With this, applications can render content into the ``head`` if they want.
Copy link
Member

Choose a reason for hiding this comment

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

reducing the amount that is rendered can be a useful optimisation

I'd present first an example of this (with controller code and renderblock for instance)

However, this optimisation prevents responses from specifying

This part can be removed i think, as the same idea is on the next sentence. And i feel it's a bit fearfull and would not want to alarm users if they dot need any meta tags (which is, as i see it, the most standard case, no ?)

With this, applications can render content into the head if they want.

Maybe something like "it allows you to set meta tags while still having a minimal ...." ?


.. code-block:: html+twig

{% extends '@Turbo/frame.html.twig' %}
Copy link
Member

Choose a reason for hiding this comment

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

  {% extends '@Turbo/frame.html.twig' %}

    {% block head %}
        <meta name="alternative" content="present" />
    {% endblock %}

    {% block body %}
        <turbo-frame id="frame_id">
            Content of the Turbo Frame
        </turbo-frame>
    {% endblock %}
<!DOCTYPE html>
<html>
  <head>
      <meta name="alternative" content="present" />
  </head>
  <body>
      <turbo-frame id="frame_id">
          Content of the Turbo Frame
      </turbo-frame>
  </body>
</html>

Not sure about this gain :)

@smnandre
Copy link
Member

smnandre commented Nov 7, 2024

Really i like the intention @seb-jean (as always with you ;) )... but i realized even the base.html / app.css / etc are not in the framework but in recipe... and you see we're not thousand around here so... ;)

@seb-jean
Copy link
Contributor Author

seb-jean commented Nov 7, 2024

Does that mean I have to delete the Turbo template?

@smnandre
Copy link
Member

smnandre commented Nov 7, 2024

1/ You don't have to anything
2/ You know better than me what could helper Turbo developpers
3/ See point #1

;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to documentation Status: Waiting Feedback Needs feedback from the author Turbo
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants