-
-
Notifications
You must be signed in to change notification settings - Fork 79k
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
Toasts: Change showing timings and classes to keep toast display:none
by default
#33610
Conversation
a4f9ec0
to
f40d64c
Compare
f40d64c
to
61ec6ba
Compare
61ec6ba
to
8b39d00
Compare
Purely from a functional point of view (whether it plays nice with AT, does what it's supposed to, etc), this seems to be working fine on https://deploy-preview-33610--twbs-bootstrap.netlify.app/docs/5.0/components/toasts/ Can't verify if this indeed closes #28752 as even on our current live docs page (without this fix) I'm not seeing the original problem - can somebody replicate the test case from that issue, using the changes from this PR, to verify? |
@patrickhlauke I may find a working example here #28752 (comment) |
ah yes ... if https://jsbin.com/gugatopiwi/1/edit?html,output is essentially using the same code as proposed here, it seems to address that problem 👍 |
8b39d00
to
572aa50
Compare
776c52c
to
5307eb6
Compare
Is this a breaking change at all? |
I'll move this to 5.0.2 so that we get more eyes on this. |
cb1f537
to
8a3a513
Compare
Unfortunately, we can't land this even if it doesn't cause any issues. People might be overwriting the CSS selectors. We can target this to v6.0.0. |
what's the breaking concern? that authors may have used |
Yup, exactly. As much as I like a good cleanup, this will break people's overwrites. :/ |
@XhmikosR As an alternative, we can leave the this would be ok? |
doubling it up for now with |
I don't think we have to leave the css functionality on hide class, as it will be handled fine by the others. |
Yeah, as long as we're not removing the class toggle entirely—and only changing the use of it in our CSS—I think we're in the clear. |
7972b8c
to
25c0e77
Compare
Did the change, added documentation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from some wording, it looks great IMHO. CSS gets kind of defensive this way 👍
7207b46
to
32b4119
Compare
display:none
by defaultdisplay:none
by default
32b4119
to
314f515
Compare
Background:
Toast Component uses
hide
to force it get adisplay: none
.This causes some strange attitude on initialized components that doesn't have
show
class, as ti overlaps page content cause of given opacity. We have also thehide
class to use in order to keep the component hidden.Solution:
We can borrow the
Collapse
component functionality, and by changing some class timings, we can avoidhide
Fixes #28752
Preview: https://deploy-preview-33610--twbs-bootstrap.netlify.app/docs/5.0/components/toasts/