-
Notifications
You must be signed in to change notification settings - Fork 38
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
remove nested toasters #2153
remove nested toasters #2153
Conversation
ad1969c
to
826cadf
Compare
9a51e2a
to
46ba2ad
Compare
46ba2ad
to
0d31173
Compare
'@itwin/itwinui-react': minor | ||
--- | ||
|
||
Nested `ThemeProvider`s will now reuse the same toaster instead of creating new ones. |
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.
Should this be "toast wrapper" instead of "toaster"?
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.
I used "toaster" because that's the terminology that our users are more familiar with, and because it's also more accurate, since we are now actually using the same reducer (rather than just the same DOM element).
Changes
This PR updates the
PortalContainer
logic so that instead of creating a new<Toaster>
for every single<ThemeProvider>
, the same one will be reused (if found).This sounds like an optimization, but it is actually a bug fix. Previously, when multiple toasts from different
toaster
s were triggered at the same time, they would end up in different containers that are positioned in the same place and therefore overlap each other. With this change, they use the same container and do not overlap.Note: This PR does not handle redundancy for the special case when a
portalContainer
prop is used; which means we still create new toasters even when differentThemeProvider
s use the sameportalContainer
(this can be seen if you inspect the<body>
element on some docs pages). This is something I'd like to fix in the future.Testing
Tested manually and added e2e tests to confirm desired behavior.
Before:
After:
For some reason, test images in
Table
andDatePicker
changed. I have no clue why. I did verify that elements are still correctly focused; the only difference is that the focus ring is sometimes not visible when using mouse (which matches how:focus-visible
works).Docs
Added
minor
changeset.