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

[Layout] Too many "forced" content for inner .page-body #213

Open
cavasinf opened this issue Feb 24, 2025 · 6 comments · May be fixed by #214
Open

[Layout] Too many "forced" content for inner .page-body #213

cavasinf opened this issue Feb 24, 2025 · 6 comments · May be fixed by #214
Labels
BC Break This will cause BC Break Bug Something isn't working Status: Reviewed Has staff reply/investigation

Comments

@cavasinf
Copy link
Collaborator

cavasinf commented Feb 24, 2025

Hey,
We’re currently a bit stuck with how the layout-xxx.html.twig file define the base 'structure' of the page.

Objective

The goal is to create a dashboard page with multiple rows and columns of with the same height.

Current state

Currently, the base template forces this HTML structure:

<div class="row row-cards">          
   <section id="" class="content">
       [...]
   </section>
</div>

<div class="{{ ''|tabler_container }}">
<div class="row row-cards">
{% block page_content_before %}{% endblock %}
<section id="{% block page_content_id %}{% endblock %}" class="{% block page_content_class %}content{% endblock %}">
{% block page_content_start %}{{ include('@Tabler/includes/flash_messages.html.twig') }}{% endblock %}
{% block page_content %}{% endblock %}
{% block page_content_end %}{% endblock %}
</section>

Render

Current render

Here's what we have currently:
Image

Desired render:

Image

To achieve this, we need to apply the .row-deck class directly after the top-level .container class.
But we want the .row-deck in the top div + remove the section part.

The whole HTML wanted:
Image

Question/Solution

  1. Is there specific reason for having the section block + default row row-cards classes?
  2. It may be a relic of admin-lte bundle?

It might be more flexible if this part of the structure was left for the developers to define, rather than enforced by the top-level template.

References with differents classes just after the .container div:
https://preview.tabler.io/
https://preview.tabler.io/activity.html
https://preview.tabler.io/photogrid.html

@cavasinf cavasinf added BC Break This will cause BC Break Bug Something isn't working Status: Needs Review Not under investigation labels Feb 24, 2025
@kevinpapst
Copy link
Owner

This was in your initial commit already:
2584dc1#diff-ad9fb94d48b0a156f12a7c3972ef9db886e2eb46f6626bafb146cd53d7d77c69R119

Who knows which feature accidentally work due to this class being existent right now.
Changing would be quite a BC break.

Anyway, a dashboard with same height widgets works perfectly fine for me with that HTML:

Image

You can simply add a second div with the wanted CSS classes and everything works fine.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Feb 25, 2025

This was in your initial commit already

Yes, I was looking for when/why we do that, it was there from the beginning:
657ea71#diff-77f98db4bbf74ae14cad716cd52d662106d4ea5a78039716a76307c9745b7be9R112-R114

Who knows which feature accidentally work due to this class being existent right now.
Changing would be quite a BC break.

Yes, it could, testing in the table preview, there is no big impact:

  1. The .content class does nothing
  2. .row .row-cards add the same margin as .page-body.
  • The only impact is if you have a js/css selector targeting this.
  • Starting a page directly with some .col without .row (because we add it in the base template)

@kevinpapst
Copy link
Owner

Do you want to sent a PR with the changes?
I will try then apply your changes locally and test for a while to see if anything weird happens.

@cavasinf
Copy link
Collaborator Author

Do you want to sent a PR with the changes?

Yes I can
Do we keep the section.content element?

@kevinpapst
Copy link
Owner

I though you only replace <div class="row row-cards"> with <div class="row">.

This PR change will definitely blow up my app. I use both blocks / the section as selector in JS and CSS:

{% block page_content_class %}{{ parent() }} {% block page_class %}{% endblock %}{% endblock %}

And I am almost certain I will find more references if I search more detailed:
Image

@cavasinf
Copy link
Collaborator Author

cavasinf commented Feb 25, 2025

I though you only replace <div class="row row-cards"> with <div class="row">.

No 😢

This PR change will definitely blow up my app

We have a project where another developer also uses/overrides these blocks.
The quick fix was to update the base.html.twig with:

{% block page_content_before %}
   <div class="row row-cards">
      <section id="{% block page_content_id %}{% endblock %}" class="{% block page_content_class %}content{% endblock %}">
{% endblock %}

{% block page_content_after %}
      </section>
   </div>
{% endblock %}

It is a temporary patch to avoid the BC break but will/should not be left that way.


The end goal it that every pages that uses {% block page_content %}{% endblock %} should look like this now:

{% block page_content %}
    <div class="row row-cards">
        <div class="col-sm-6 col-lg-3"></div>
        <div class="col-sm-6 col-lg-3"></div>
        <div class="col-sm-6 col-lg-3"></div>
        <div class="col-sm-6 col-lg-3"></div>
    </div>
{% endblock %}
{% block page_content %}
    <div class="col-sm-6 col-lg-3"></div>
    <div class="col-sm-6 col-lg-3"></div>
    <div class="col-sm-6 col-lg-3"></div>
    <div class="col-sm-6 col-lg-3"></div>
{% endblock %}

OR

{% block page_content %}
    <div class="row justify-content-center">
    	<div class="col-8">
             <div class="card"></div>
    	</div>
    </div>
{% endblock %}
{% block page_content %}
    <div class="justify-content-center">
    	<div class="col-8">
             <div class="card"></div>
    	</div>
    </div>
{% endblock %}

@cavasinf cavasinf added Status: Reviewed Has staff reply/investigation and removed Status: Needs Review Not under investigation labels Feb 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BC Break This will cause BC Break Bug Something isn't working Status: Reviewed Has staff reply/investigation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants