[Alert] Add id and attr option

[cavasinf]

Description

Adds id and attr option

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]

Should we remove the deprecated part now @kevinpapst ?

[kevinpapst]

Yes, remove the deprecated stuff.

I know you like the better readability of multiline HTML.
But I have several test cases where I check the generated HTML for special cases (sometimes that is the only way to test a certain code piece. Those tests always break with HTML structure changes. Can you use {%- and -}} to prevent line breaks?

What just came to my mind while reading your changes: we could merge role and id into the _attr array and let the macro create all HTML attributes. The macro could prevent line breaks, my tests are happy about the HTML and you have the better readability.

[cavasinf]

Yes, remove the deprecated stuff.

Done ✔️

I know you like the better readability of multiline HTML.

Yes I do 😉 And I do it without noticing it now, sorry 😅

But I have several test cases where I check the generated HTML for special cases (sometimes that is the only way to test a certain code piece. Those tests always break with HTML structure changes. Can you use {%- and -}} to prevent line breaks?

Good idea! Done ✔️

What just came to my mind while reading your changes: we could merge role and id into the _attr array and let the macro create all HTML attributes.

Funny, because I was thinking about that when I added those new params.
IT IS a great idea, but then I thought of other includes/component/... If we're going to do this (which I'm 💯 percent with). We should do it in a new PR that changes all those things and warn devs about this BC break.

[cavasinf]

Talking about things to change.
I don't think that:

{% set _background = _title is empty or _description is empty or _important %}

Has its place in the bundle.
Maybe it is related to your logic or the old AdminLTE logic.
But I think the alert-important class should be written when the important option is set to true.
Not with magic variable that can be tricky.

WDYT ?

We can deprecate it like that:

{% if (_title is empty or _description is empty) and options.important is not defined %}
    {% deprecated "Using important alert without setting the 'important' option is deprecated, set option to true instead." %}
    {% set _important = true %}
{% endif %}

[kevinpapst]

Where do I find {% set _background = _title is empty or _description is empty or _important %} that?

I had recently troubles showing an alert notification in a certain way. Maybe that was the reason...

[cavasinf]

Where do I find {% set _background = _title is empty or _description is empty or _important %} that?

I had recently troubles showing an alert notification in a certain way. Maybe that was the reason...

There:

TablerBundle/templates/components/alert.html.twig

Line 19 in 4148b24

{% set _background = _title is empty or _description is empty or _important %}

[cavasinf]

Any update on that @kevinpapst?

[kevinpapst]

Tested, works 👍 merge 🚀

Thanks!