Refactoring alert() macro

[cavasinf]

Description

Related to #89

Usage

{% from '@Tabler/components/alert.html.twig' import alert %}

<div class="row">
    <div class="col-6">
        {{ alert('danger', 'Description' ,'Title', 'fas fa-exclamation-triangle', true) }}
        {{ alert('danger', 'Description' ,'Title', 'fas fa-exclamation-triangle') }}
        {{ alert('danger', 'Description' ,'Title', 'fas fa-exclamation-triangle') }} {# Error here #}
        {{ alert('warning', 'Description' ,'Title') }}
        {{ alert('warning', null ,'Title') }}
        {{ alert('warning', null ,'Title', 'fas fa-exclamation-triangle') }}
        {{ alert('success', 'Description') }}
        {{ alert('success', 'Description', null, 'fas fa-exclamation-triangle') }}
        {{ alert('primary') }}
    </div>

    <div class="col-6">
        {{ alert({title : 'Title V2', description: 'Description V2', icon : 'warning', dismissible : true}) }}
        {{ alert({title : 'Title V2', description: 'Description V2', icon : 'warning'}) }}
        {{ alert({title : 'Title V2', description: 'Description V2', icon : 'warning', important: true}) }} {# Looks good now #}
        {{ alert({title : 'Title V2', description: 'Description V2', type : 'warning'}) }}
        {{ alert({title : 'Title V2', type : 'warning'}) }}
        {{ alert({title : 'Title V2', icon : 'warning', type : 'warning'}) }}
        {{ alert({description : 'Description V2', type : 'success'}) }}
        {{ alert({description : 'Description V2', type : 'success', icon : 'warning'}) }}
        {{ alert({type : 'primary'}) }}
    </div>
</div>

Preview

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

[kevinpapst]

Can you make the raw filter optional by a setting like in my example.
This filter can easily lead to CSRF issues if you pass user generated values to it.
Better activate it per case, instead of "blindly" using unescaped data.

[cavasinf]

Sure 👍

[kevinpapst]

nice 👍 ready for merge from your end?

[cavasinf]

Everything's OK for me 👌 Le mar. 31 mai 2022, 18:05, Kevin Papst ***@***.***> a écrit :

…

nice 👍 ready for merge from your end? — Reply to this email directly, view it on GitHub <#90 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGA7DBXOD3GBTBJAVPLJQI3VMY2DTANCNFSM5XNPTRPQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[kevinpapst]

I will add your examples to the demo application. Thanks you @cavasinf 👍

[cavasinf]

Yeah I haven't seen a doc for that kind of feature.
We can maybe add the doc of that component is this repo like others 🤔

[kevinpapst]

This is what I see after the merge:

Can you confirm that is still looks as in your screenshot?

[kevinpapst]

Sorry, the icon warning wasn't configured.

[kevinpapst]

Yeah I haven't seen a doc for that kind of feature.

Because you still don't use the demo app, do you 😁

[kevinpapst]

Okay, there is one BC break which is not matching the Tabler docs: https://preview.tabler.io/docs/alerts.html

Before:

After:

Could you have a look while I work on the docs?

[cavasinf]

@kevinpapst Looks like it's coming from the icon class again.

Tabler user SVG, we use FontAwesome.
Can you try with false param at tabler_icon twig function.

TablerBundle/templates/components/alert.html.twig

Line 34 in da494e8

{{ tabler_icon(_icon, true) }}

I remember it was "odd" without this class, but if it's okay for you, we can switch that 👍

[cavasinf]

I'll check for title color + description => new PR ?

[kevinpapst]

I don't mind the icon size/positioning, bigger looks better and I didn't even recognize it before you mentioned it ;-)

I was only talking about the colors, which worked before. New PR sounds good :)

[kevinpapst]

Going through my deprecations log, I saw that we missed one:
https://github.com/kevinpapst/TablerBundle/blob/main/templates/components/callout.html.twig#L3

callout is calling alert with single params 🙄

[cavasinf]

What do you want to do ?
Add the deprecated message and change multi params into one object param.

Also, why it has 6 parameters for a macro that can only have 5 ?

[kevinpapst]

Because important is the 6 param and it got lost somewhere in the refactoring process.
To have a consistent API we should change the signature as well:

{% from '@theme/components/alert.html.twig' import alert %}
{% if type is iterable %}
    {{ alert(type) }}
{% else %}
    {% deprecated "Passing multiple parameters to 'callout()' Tabler macro is deprecated, use options object syntax." %}
    {{ alert({type: type|default('danger'), description: description, title: title|trans, icon: icon, dismissible: false, important: true}) }}
{% else %}

What do you think ?

[cavasinf]

NVM

[cavasinf]

• Missing the trans of description (like OLD alert macro)
• icon is required in callout macro ?
• description is required ?
• Maybe add {% set options = type %} inside if condition (if devs looks at code).
• Careful at "end if" block 😉