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

Move blocks from layout from base_layout #1593

Closed

Conversation

eerison
Copy link
Contributor

@eerison eerison commented Sep 5, 2022

Subject

I am targeting this branch, because {reason}.

Closes #{put_issue_number_here}.

Changelog

### Added
- Added some `Class::newMethod()` to do great stuff.

### Changed

### Deprecated

### Removed

### Fixed

### Security

To do

  • Update the documentation;
  • Add an upgrade note.
  • After the changes is approved, I'll do a commit fixing the indentation

@eerison eerison marked this pull request as draft September 5, 2022 13:58
@eerison eerison force-pushed the remove_deprecate_twig_tags branch from 9184894 to 7164687 Compare September 5, 2022 14:10
@eerison eerison force-pushed the remove_deprecate_twig_tags branch from 7164687 to d122d61 Compare September 5, 2022 14:21
@SonataCI
Copy link
Collaborator

SonataCI commented Sep 6, 2022

Could you please rebase your PR and fix merge conflicts?

@eerison
Copy link
Contributor Author

eerison commented Sep 6, 2022

it's the example how the user should extend the layout.html.twig

file: templates/bundles/SonataPageBundle/layout.html.twig

{% extends '@SonataPage/base_layout.html.twig' %}

{% block sonata_page_stylesheets %}
    <link rel="stylesheet" href="your.css" media="all">
{% endblock %}

{% block sonata_page_body %}
    <header>
        {{ block('sonata_page_container_header') }}
    </header>

    <section>
        {{ block('sonata_page_container_breadcrumb') }}
    </section>

    {% block sonata_page_container_content_top %}
        {% if page is defined %}
            <section>
                {{ parent() }}
            </section>
        {% endif %}
    {% endblock %}

    <section>
        {{ block('sonata_page_container_content') }}
    </section>


    {% block sonata_page_container_content_bottom %}
        {% if page is defined %}
            <section>
                {{ parent() }}
            </section>
        {% endif %}
    {% endblock %}

    <footer>
        {{ block('sonata_page_container_content_footer') }}
    </footer>
{% endblock %}

@eerison
Copy link
Contributor Author

eerison commented Sep 6, 2022

the user override the layout like this

file: templates/bundles/SonataPageBundle/layout.html.twig

{% extends '@SonataPage/base_layout.html.twig' %}

{% block sonata_page_stylesheets %}
    <link rel="stylesheet" href="your.css" media="all">
{% endblock %}

{% block sonata_page_body %}
    <header>
        {{ block('sonata_page_container_header') }}
    </header>

    <section>
        {{ block('sonata_page_container_breadcrumb') }}
    </section>

    {% block sonata_page_container_content_top %}
        {% if page is defined %}
            <section>
                {{ parent() }}
            </section>
        {% endif %}
    {% endblock %}

    <section>
        {{ block('sonata_page_container_content') }}
    </section>


    {% block sonata_page_container_content_bottom %}
        {% if page is defined %}
            <section>
                {{ parent() }}
            </section>
        {% endif %}
    {% endblock %}

    <footer>
        {{ block('sonata_page_container_content_footer') }}
    </footer>
{% endblock %}

Hey @jordisala1991 what do you think we move the blocks created into the layout.html.twig to base_laout.html.twig?

this way I can reuse the code when I'm overriding the layout.html.twig

@jordisala1991
Copy link
Member

IMO we should split PRs. If you are removing deprecated blocks, thats one PR. If you are adding new blocks, thats another PR. To me the only one needed to release is to remove deprecated blocks, but if you want to add more blocks I won't block that PR. (but please, we do separate to see what is new and what is removed better).

@SonataCI
Copy link
Collaborator

SonataCI commented Sep 6, 2022

Could you please rebase your PR and fix merge conflicts?

@eerison
Copy link
Contributor Author

eerison commented Sep 6, 2022

IMO we should split PRs. If you are removing deprecated blocks, thats one PR. If you are adding new blocks, thats another PR. To me the only one needed to release is to remove deprecated blocks, but if you want to add more blocks I won't block that PR. (but please, we do separate to see what is new and what is removed better).

Ok, I'll let this PR for this new blocks and after that I rebase with 4.x.

@eerison
Copy link
Contributor Author

eerison commented Sep 6, 2022

This PR is blocked by #1594

@eerison eerison changed the title Deprecate twig tags Move blocks from layout from base_layout Sep 6, 2022
@eerison
Copy link
Contributor Author

eerison commented Sep 8, 2022

well, and about this iPR

make sense we overwrite the layout to work like this? or should we use sonata functions directly into the extended template (in the user side)

@eerison
Copy link
Contributor Author

eerison commented Sep 8, 2022

The main motivation for this PR is the user avoid to implement things like this

                    {% if content is defined %}
                        {{ content|raw }}
                    {% else %}
                        {% set content = block('content') is defined ? block('content') : '' %}
                        {% if content|length > 0 %}
                            {{ content|raw }}
                        {% elseif page is defined %}
                            {{ sonata_page_render_container('content', page) }}
                        {% endif %}
                    {% endif %}

@jordisala1991
Copy link
Member

Is this PR a bugfix for 4.x (for a thing that was working on 3.x?), or does this PR breaks BC in a way that it can't be done in a minor?

If any of those 2 question the answer is "Yes", and the PR is simple enough, let's do it, otherwise let's wait after 4.0

@eerison
Copy link
Contributor Author

eerison commented Sep 13, 2022

I'll reopen this PR when I back to work on this ;)

@eerison eerison closed this Sep 13, 2022
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