Alert close button API is incompatible with previous versions about yii2-bootstrap5 HOT 14 CLOSED

Interesting. We need to re-check how https://github.com/yiisoft/yii-bootstrap5 actually works in this case. It's not released yet so could contain bugs.

from yii2-bootstrap5.

At no point did I intend for this to get so drawn out, trust me — I was hoping for the first response comment of the issue to be something like "oh, right, we missed this/didn't think of that; sure we're gonna fix this".

I understand what you mean. If it would be documented, or having the same behaviour as in previous versions, this case got never opened.

ISTM I have failed to present the point of the test. Here, I'll try to explain: the test verifies a single, very specific feature, behavior that is shared by pretty much all versions of Yii Bootstrap extension, previous and upcoming, this one being the odd one out.

In previous versions of bootstrap, the "icon" always got rendered by content: bs2, bs3, bs4. This is the first one who changed this behavior, why I changed it too.
But I understand your point.
My proposal: bring back the "label" option and explicitly mark it as deprecated in docs and add some hints about how to use it (maybe with @antichris' example).
@bizley, @samdark, @antichris: what do you think about? Can everyone live with it?

from yii2-bootstrap5.

I think that we should have it and it should not be deprecated. It will allow more customization like @antichris mentioned.

from yii2-bootstrap5.

Alright. Let's have it:

1. To ease transition.
2. It's useful overall.

from yii2-bootstrap5.

According docs there is no possibility to change close label any more. The ❌ icon will get rendered by background property, so it's going to look strange if you'd have any content in it. The type="button" can get overriden by e.g.

Alert::widget([
    'closeButton' => [
        'tag' => 'a',
        'type' => ''
    ]
]);

from yii2-bootstrap5.

According docs there is no possibility to change close label any more.

Sorry, there must be something wrong with my internet or eyes, but I just could not find it. Could you just paste here a direct quotation where it says that?

The type="button" can get overriden by [setting the attribute value to an empty string].

Why do you think this is an example of good API design?

from yii2-bootstrap5.

It's nowhere directly mentioned, but try it yourself. First their example:

<div class="alert alert-warning alert-dismissible fade show" role="alert">
  <strong>Holy guacamole!</strong> You should check in on some of those fields below.
  <button type="button" class="btn-close" data-bs-dismiss="alert" aria-label="Close"></button>
</div>

There is no content inside button tag. Have a look what it looks like if you insert some content:

I inserted a red × here, now you have × twice. One by background, one by content.

Why do you think this is an example of good API design?

It's not, you're right. It's something to be reconsidered to work over.

from yii2-bootstrap5.

Sure:

<div class="alert alert-warning alert-dismissible fade show" role="alert">
  <strong>Holy guacamole!</strong> You should check in on some of those fields below.
  <button type="button" class="btn btn-outline-warning" data-bs-dismiss="alert"
    style="position:absolute;top:.5rem;right:.5rem">Dismiss</button>
</div>

And this is what I got:

The damn thing works! So, care to elaborate, why should this be made impossible by the design of the API?

from yii2-bootstrap5.

The damn thing works! So, care to elaborate, why should this be made impossible by the design of the API?

Of course it works. With enough customizing everything works. There is no example in any component in the docs to customize the close button, so there is no guarantee it will still work when there is a bootstrap update. So I think it should be discussed: @bizley, @samdark your opinion?

from yii2-bootstrap5.

With enough customizing everything works.

Right. But the current design of Alert precludes any such customization — apart from going and plopping the markup for a close button in the body of the alert by hand. It's exactly what this issue is all about.

I'm doubtful there is much added value in discussing this. Simply take what's been done in the next-gen Yii "3" Bootstrap 5 extension, and replicate it here.

from yii2-bootstrap5.

@antichris please tone down you snarky comments and keep the discussion constructive, you are not really helping your cause like that.

I don't necessarily agree that this package must behave the same way as the previous versions. And showing some arbitrary test as a proof that we have similarities in few cases and thus all differencies are bad is just pure logical error.

But...

Yii 2 users are pretty used to the way previous versions of widgets work and we should made it easier for them to move to this version. If some elements of the widgets were used then and it is possible to have them in the v5 version (even though BS5 is not showing them anymore in the examples) we should have them here as well. This is the best moment for such BC adjustments because this package is still fresh.

from yii2-bootstrap5.

snarky comments

So, you disagree that presentation concerns should be separated from domain logic, and that PHP is, in fact, a legacy freakshow of language that's wheezing for a final shot in its severely misshapen head? That's okay, I'm perfectly fine with us not sharing the same sense of humor, and your opinion not reflecting mine.

I don't necessarily agree that this package must behave the same way as the previous versions.

As I see it, https://github.com/yiisoft/yii-bootstrap5 has been ignored every time I've brought it up. Are you saying that is also a "previous version" and I've been wrong about it all along?

some arbitrary test as a proof that we have similarities in few cases and thus all differencies are bad

ISTM I have failed to present the point of the test. Here, I'll try to explain: the test verifies a single, very specific feature, behavior that is shared by pretty much all versions of Yii Bootstrap extension, previous and upcoming, this one being the odd one out.

It does not just prove "similarities in [a] few cases". It tests an actual, specific behavior that was implemented by someone (AFAICT, it was here) who wrote those lines on purpose (I assume, code don't often happen by accident), it is depended upon by users, and is missing here from this one, single version of Yii Bootstrap extension.

Yii 2 users are pretty used to the way previous versions of widgets work and we should made it easier for them to move to this version.

... Is exactly what I've been saying all along. Not those exact words, maybe — sorry, English is not my first language.

At no point did I intend for this to get so drawn out, trust me — I was hoping for the first response comment of the issue to be something like "oh, right, we missed this/didn't think of that; sure we're gonna fix this".

I would even have missed this bug (or call it whatever you want) myself, if I didn't have a project based on one of the Yii 2 application templates that had that extended Alert widget, the one using session flash messages, covered with waterproof tests. Upgrading the project to this extension, the test case that verifies that those extended alerts can have customized close buttons failed.

The most ridiculous part is that I've never actually used customized alert close buttons for anything. Mostly that may be because I'm a systems/back-end engineer, and I sincerely could not give a damn less about how something appears as long as it does not get in the way of how something (or someone) works.

from yii2-bootstrap5.

I think that we should check yiisoft/yii-bootstrap5#53 first.

from yii2-bootstrap5.

Given

Alert::widget([
    'body' => "Low Blow: Bob Loblaw's Law Blog Lobs Law Bomb",
    'options' => ['class' => 'alert-warning'],
    'closeButton' => [
        'label' => 'Dismiss',
        'tag' => 'a',
        'class' => ['buttonOptions' => 'btn btn-outline-warning'],
        'style' => 'position:absolute;top:.5rem;right:.5rem',
    ]
]);

yields

<div id="w0" class="alert-warning alert alert-dismissible" role="alert">

Low Blow: Bob Loblaw's Law Blog Lobs Law Bomb
<a type="button" class="btn btn-outline-warning" data-bs-dismiss="alert" aria-label="Close" label="Dismiss" style="position:absolute;top:.5rem;right:.5rem"></a>

</div>

The funky whitespace is consistent with previous versions, the <a> tag's attribute type="button" and lack of content is not. Compare with yiisoft/yii-bootstrap5#53 (comment).

from yii2-bootstrap5.