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

[Card] Add .card-body print condition #155

Merged
merged 6 commits into from
Apr 6, 2023
Merged

Conversation

cavasinf
Copy link
Collaborator

@cavasinf cavasinf commented Mar 31, 2023

Description

A Card with a fullsized table from Tabler should apparently not be inside card-body.

See here for Tabler demo: https://preview.tabler.io/tables.html (see invoices table)
image

With empty body and no padding (fullsize card template option) + Table inside card-body
image

I've added condition to not always print this part.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I updated the documentation (see here)
  • I agree that this code will be published under the MIT license

@cavasinf cavasinf added Bug Something isn't working Status: Needs Review Not under investigation BC Break This will cause BC Break labels Mar 31, 2023
@cavasinf cavasinf added this to the 1.0 milestone Mar 31, 2023
@cavasinf cavasinf changed the title Add card-body condition [Card] Add card-body condition Mar 31, 2023
@kevinpapst
Copy link
Owner

This exact use-case is the reason, why I added the box_type_class , so you can swap card-body with card-table.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Mar 31, 2023

I'm already using card-table on the table class, which is wrapped by table-responsive.
Even if I double this class (to replace the card-body), I still have this double border on the footer:

image

And without the first card-table (so no card-body either):
image

Maybe I'm doing it wrong, or this is not the way to go

@kevinpapst
Copy link
Owner

Bildschirm­foto 2023-03-31 um 22 32 16

Can you try that:

        {% embed '@theme/embeds/card.html.twig' %}
            {% from "macros/widgets.html.twig" import work_times_result %}
            {% block box_type_class %}card-table table-responsive {% endblock %}
            {% block box_body %}
                <table class="table table-vcenter table-hover dataTable">

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 4, 2023

Yeah it works!
But why do a simple div .card-table as parent won't..

I have:

  1. A _list.html.twig partial file that is composed of that table:
<div class="table-responsive">
    <table class="table table-vcenter">
        <tbody>
        {# ... #}
        </tbody>
    </table>
</div>
  1. An index.html.twig that call that partial inside a .card (this DO NOT WORK):
{% embed '@Tabler/embeds/card.html.twig' with {fullsize: true} %}
    {% block box_type_class %}card-table{% endblock %}
    {% block box_body %}
        {% include 'produit/_partials/_list.html.twig' with {produits : paginator.results} %}
    {% endblock %}
{% endembed %}
  1. A modal that includes directly the _list.html.twig inside the modal body (not a card).
<div class="modal" tabindex="-1">
  <div class="modal-dialog">
    <div class="modal-content">
      <div class="modal-header">
        {# ... #}
      </div>
      <div class="modal-body">
        {% include 'produit/_partials/_list.html.twig' with {produits : paginator.results} %}
      </div>
      <div class="modal-footer">
        {# ... #}
      </div>
    </div>
  </div>
</div>

(3) I don't want to have the .card-table class when using the modal.
(1) Adding that class inside the _list template sounds wrong.
(1) And conditionally add the class inside the _list sounds lazy too.

It is maybe a Tabler issue with how that class behavior works 🤔

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 4, 2023

I didn't post the solution..

Here's how it should be, to work today:

  1. _list.html.twig:
- <div class="table-responsive">
    <table class="table table-vcenter">
        <tbody>
        {# ... #}
        </tbody>
    </table>
- </div>
  1. index.html.twig:
{% embed '@Tabler/embeds/card.html.twig' %}
-   {% block box_type_class %}card-table{% endblock %}
+   {% block box_type_class %}card-table table-responsive{% endblock %}
    {% block box_body %}
        {% include 'produit/_partials/_list.html.twig' with {produits : paginator.results} %}
    {% endblock %}
{% endembed %}
  1. A modal that includes directly the _list.html.twig inside the modal body (not a card).
<div class="modal" tabindex="-1">
  <div class="modal-dialog">
    <div class="modal-content">
      <div class="modal-header">
        {# ... #}
      </div>
      <div class="modal-body">
+        <div class="table-responsive">
          {% include 'produit/_partials/_list.html.twig' with {produits : paginator.results} %}
+        </div>      
      </div>
      <div class="modal-footer">
        {# ... #}
      </div>
    </div>
  </div>
</div>

@cavasinf cavasinf removed the BC Break This will cause BC Break label Apr 4, 2023
cavasinf

This comment was marked as duplicate.

@cavasinf cavasinf added Feature Feature requested and removed Bug Something isn't working labels Apr 4, 2023
@cavasinf cavasinf changed the title [Card] Add card-body condition [Card] Add print card-body condition Apr 4, 2023
@cavasinf cavasinf changed the title [Card] Add print card-body condition [Card] Add .card-body print condition Apr 4, 2023
@kevinpapst
Copy link
Owner

The HTML setup as used by me and what I posted previously comes directly from preview.tabler.io.
That's how the classes are composed.
If you add that extra block around, you don't have to use workarounds with body_before or body_after. What do you think?

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 4, 2023

comes directly from preview.tabler.io.

Yeah, I've also searched for how they intend to use it.
Looking at the code, they have no real "way" to use it:

  • Can be on .table
  • Can be on .table when using .table-responsive
  • Can be directly on .table-responsive

See here: https://github.com/tabler/tabler/search?q=card-table&type=code

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 4, 2023

If you add that extra block around, you don't have to use workarounds with body_before or body_after. What do you think?

No I don't think so..

Once again, I'm following Tabler of "How you should do it".
I've made a card-paginator template that allows me to:

  1. quick filter
  2. number of entries on a page
  3. paginate a table card element

That homemade card extends "TablerBundle card" and uses those xxx_before and/or xxx_after to follow Tabler ways:

image

See here "invoices" table

ATE, this is why I've made this PR 😄 to be able to replicate exactly that card template.

@kevinpapst
Copy link
Owner

That is IMO stupid from Tabler side.

What if you:

  • move the search part inside block box_body_before with the classes border-bottom p-3
  • use the new block box_type_class with the content card-body table-responsive
  • and then move the main table into block body

Wouldn't that work?

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 5, 2023

That is IMO stupid from Tabler side.

😆

Wouldn't that work?

Maybe, but I'm brainlessly following what they say "how to do".
If the doc (there is no doc here 😉) says to do it that way, I'll do it that way, "stupid" or not 😆.
And I think other developers will too, especially if they are not familiar with "front things".

To add more complexity, Tabler really likes to put + condition inside there selectors.

  • Breadcrumb
  • Buttons
  • Button Group
  • List group
  • Double Card Body
  • Card button
  • Nav Tabs
  • And more..

This means that if we don't stick to how they intend it, and move a class or wrap it with another div,
one/several whole selectors are out.
(I've done this to a custom component, and it's a lot of pain to keep alive when updating the template...)

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 5, 2023

Side note: I also use card with multiple bodies. And Tabler is okay with that.
https://github.com/tabler/tabler/blob/dev/dist/css/tabler.css#L18916-L18918

image

@kevinpapst
Copy link
Owner

Can you please give it a try?
The card component is already terrible as is. If we squeeze more and more if/else into it, it doesn't get better.
And the way I am proposing is also from the preview page, I didn't make it up myself.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 6, 2023

Can you please give it a try?

Sure!
And it works:
image

That is IMO stupid from Tabler side.

I join you on that point, it is more natural to me to do it that way..

ATE there is:

  • box_header block content
  • `box_header_after' block content
  • No box body:
    1. Can be removed using box_type_class
    2. (With this PR) No box_body block + use block_body_before OR block_body_after block.
  • Content of the box_footer block

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 6, 2023

TBH, I really don't like box_type_class:

  1. I'll never look to that name to remove a .box-body class.
  2. boxtype param is already used to define the box status (which I don't like either)

I'd rather have a box_body_classes that wraps each box_body class.
Or (this PR) not printing the body at all.


I can give other examples that I use today.

1. If I want to do a ribbon without a card body:

<div class="ribbon ribbon-top bg-yellow">
   <!-- Content -->
</div>

I'll not replace the box_type_class to add ribbon classes.
My though will be to not print the body and place my custom ribbon after the header or before the body.

<div class="card">
  <div class="card-header">
    <h3 class="card-title">Card title</h3>
  </div>
  <div class="ribbon ribbon-top bg-yellow">
    <svg></svg>
  </div>
  <!-- NO body-->
</div>

image

2. Same case for a card with a progress bar .card-progress

<div class="progress progress-sm card-progress">
  <div class="progress-bar" style="width: 38%" role="progressbar" aria-valuenow="38" aria-valuemin="0" aria-valuemax="100" aria-label="38% Complete">
    <span class="visually-hidden">38% Complete</span>
  </div>
</div>

After the body block OR before the footer block

<div class="card">
  <div class="card-header">
    <h3 class="card-title">Card title</h3>
  </div>
  <!-- NO body-->
  <div class="progress progress-sm card-progress">
    <div class="progress-bar" style="width: 38%" role="progressbar" aria-valuenow="38" aria-valuemin="0" aria-valuemax="100" aria-label="38% Complete">
      <span class="visually-hidden">38% Complete</span>
    </div>
  </div>
</div>

image

WDYT?

@kevinpapst
Copy link
Owner

The name is not ideal but box_body_class was already used and moving the card_body class into box_body_class would have broken all of our apps. That why I went with box_type_class. Not great naming probably... but I only accept criticism, if you come up with a better alternative 😁 and we can still change it, I doubt that it is widely used yet.

That's why I proposed a block around the body div, so you can easily empty/remove it. But with the current setup, you should be able to use the box_type_class to achieve both your progress:

{% block box_type_class %}progress progress-sm card-progress{% endblock %}
{% block box_body %}
  <div class="progress-bar" style="width: 38%" role="progressbar" aria-valuenow="38" aria-valuemin="0" aria-valuemax="100" aria-label="38% Complete">
    <span class="visually-hidden">38% Complete</span>
  </div>
{% endblock %}

and ribbon example:

{% block box_type_class %}ribbon ribbon-top bg-yellow{% endblock %}
{% block box_body %}
    <svg></svg>
{% endblock %}

And with all the learnings from the card monster, we can create a card-v2 embed later that cleans up that "creative" mess.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 6, 2023

I'd rather have a box_body_classes that wraps each box_body class.

you don't like that one?

@kevinpapst
Copy link
Owner

Ah, I missed that one. Your posts are always so long 🙈

You mean like that:

<div class="{% block box_body_classes %}card-body {% block box_body_class %}{% endblock %}{% endblock %}

?
Looks good. I should have come up with that. Yes, lets do it and add the block around the entire body, so it can be removed.

@cavasinf
Copy link
Collaborator Author

cavasinf commented Apr 6, 2023

Your posts are always so long 🙈

Yeah I don't know why 😆
French people like to talk 🤔 Maybe..


<div class="{% block box_body_classes %}card-body {% block box_body_class %}{% endblock %}{% endblock %}

Looks good, but here's why I did that PR.
What do we do for others classes ?

  • fullsize param
  • collapisble param
{% if _fullsize %}p-0{% endif %} {% if _collapsible %} {{ _collapsible_class ~ (_collapsed ? ' collapse' : ' show') }} {% endif %}"

Do I need the fullsize condition if I want to do a ribbon?
Do I need the collapse condition if I want to do a ribbon?

So why using box_body block at all if i want to remove everything?

I know I'm a pain in the ass, but I'd rather we all agree than skipping things. (sorry 😝)

@kevinpapst
Copy link
Owner

And here is why I proposed the block instead of a variable: you have more control for yet unknown use-cases. I bet at some point one of us wants to use the body and put something entirely different in there.

Anyway ... looks good to me now after the discussion! And I will release it directly after merging, as I need to fix my uses of the box_body_type class.

@kevinpapst kevinpapst merged commit 570cdda into main Apr 6, 2023
@kevinpapst kevinpapst deleted the cavasinf-card-body branch April 6, 2023 16:30
@kevinpapst
Copy link
Owner

released 0.15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Feature requested Status: Needs Review Not under investigation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants