search

thednp / bootstrap.native

Bootstrap components build with Typescript
http://thednp.github.io/bootstrap.native/
MIT License
1.7k stars 177 forks source link

Alert is closed only if contains "show" class #407

Closed garak closed 3 years ago

garak commented 3 years ago

See https://github.com/thednp/bootstrap.native/blob/master/src/components/alert-native.js#L49

I don't understand why there's a check for a "show" class. Moreover, I can't find a mention for such class in docs

thednp commented 3 years ago

Some checks have to be in place to prevent triggering events when not make sense.

garak commented 3 years ago

I see, but don't you think that such additions should be highlighted in documentation?

thednp commented 3 years ago

The .close() method should only work when alert is present in the DOM and visible to the user. Period.

This thing does exactly the same. https://github.com/twbs/bootstrap/blob/main/js/src/alert.js#L58-L60

garak commented 3 years ago

The .close() method should only work when alert is present in the DOM and visible to the user. Period.

The point is that alert can be visible to the user even when "show" class is not present.

This thing does exactly the same. https://github.com/twbs/bootstrap/blob/main/js/src/alert.js#L58-L60

Indeed, Boostrap does not require any "show" class to close an alert. This issue is just the result of struggling with an alert not closing until I realized this strange requirement.

thednp commented 3 years ago

The point is that alert can be visible to the user even when "show" class is not present.

That is indeed not bootstrap.

garak commented 3 years ago

That is indeed not bootstrap.

Well, it's you that mentioned "this thing does exactly the same" (by linking bootstrap source) Anyway, my question stands: don't you think that documentation should report such requirement?

thednp commented 3 years ago

No I mean THIS IS NOT BOOTSTRAP

The point is that alert can be visible to the user even when "show" class is not present.

The demo documentation + wiki only showcase differences.

thednp commented 3 years ago

The original bootstrap component only considers that checking presence of the element in the DOM is enough to prevent execution. My original concern was to prevent the transitionend event from never executing because there was no "show" class to remove and enable the transition.

I think this behavior is more healthy for production websites. Frankly this is also what you should expect from it.

Meanwhile I got busy with other stuff, don't afford the time to check every line of JS to write docs, so take it as is.

garak commented 3 years ago

Well. I see that you care enough to explain that differences exist with Boostrap, and that "for each component these details are fully explained" (written in the FAQs). I guess that this is a difference it's worth mentioning, for all newcomers.

thednp commented 3 years ago

I said clearly "it's not a perfect drop-in replacement", should I make it bold?

Also this from the FAQs.

Very loud and clear.

garak commented 3 years ago

I said clearly "it's not a perfect drop-in replacement", should I make it bold?

I never questioned it. My only concern is how easy is supposed to be such replacement process. If you agree that this case is worth to be mentioned in docs, I can propose a PR.

thednp commented 3 years ago

Might worth a try, why not.