Skip to content
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

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

GeoSot
Copy link
Member

@GeoSot GeoSot commented Apr 11, 2021

Background:

Toast Component uses hide to force it get a display: 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 the hide 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 avoid hide

Fixes #28752

Preview: https://deploy-preview-33610--twbs-bootstrap.netlify.app/docs/5.0/components/toasts/

alpadev
alpadev previously approved these changes Apr 26, 2021
@GeoSot GeoSot requested a review from mdo April 27, 2021 11:55
@patrickhlauke
Copy link
Member

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?

@GeoSot
Copy link
Member Author

GeoSot commented May 6, 2021

@patrickhlauke I may find a working example here #28752 (comment)

@patrickhlauke
Copy link
Member

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 👍

@twbs twbs deleted a comment from tungphan995 May 12, 2021
@mdo
Copy link
Member

mdo commented May 13, 2021

Is this a breaking change at all?

@XhmikosR
Copy link
Member

I'll move this to 5.0.2 so that we get more eyes on this.

@GeoSot GeoSot force-pushed the gs-fix-toast-hide-issue branch 2 times, most recently from cb1f537 to 8a3a513 Compare May 18, 2021 23:32
@XhmikosR XhmikosR dismissed stale reviews from mdo and alpadev June 14, 2021 17:21

BC

@XhmikosR
Copy link
Member

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.

@patrickhlauke
Copy link
Member

what's the breaking concern? that authors may have used class="show hide" together, and the new CSS selectors (which now do away with .hide) won't kick in correctly?

@XhmikosR
Copy link
Member

Yup, exactly. As much as I like a good cleanup, this will break people's overwrites. :/

@GeoSot
Copy link
Member Author

GeoSot commented Jun 14, 2021

@XhmikosR As an alternative, we can leave the hide class to be toggled for backwards compatibility
(And write on docs that it is deprecated)

this would be ok?

@patrickhlauke
Copy link
Member

doubling it up for now with &.hide, &:not(.show) { ... } etc would indeed avoid breakage? i think it's important to fix the bug soon, rather than waiting for 6.0

@GeoSot
Copy link
Member Author

GeoSot commented Jun 14, 2021

I don't think we have to leave the css functionality on hide class, as it will be handled fine by the others.
Only the js class toggling, in case someone has based styling over its existance

@mdo
Copy link
Member

mdo commented Jun 16, 2021

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.

@GeoSot
Copy link
Member Author

GeoSot commented Jun 16, 2021

Did the change, added documentation

Copy link
Member

@ffoodd ffoodd left a 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 👍

site/content/docs/5.0/components/toasts.md Outdated Show resolved Hide resolved
@XhmikosR XhmikosR requested a review from mdo July 16, 2021 06:44
@XhmikosR XhmikosR changed the title Toast.js: Change Showing timings and classes to keep toast display:none by default Toasts: Change showing timings and classes to keep toast display:none by default Jul 22, 2021
@XhmikosR XhmikosR merged commit 41292a5 into main Jul 22, 2021
@XhmikosR XhmikosR deleted the gs-fix-toast-hide-issue branch July 22, 2021 15:13
marvin-hinkley-vortx pushed a commit to Vortx-Inc/bootstrap that referenced this pull request Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Initialised (but not visible) toast overlaps page content (preventing clicks on page)
7 participants