-
Notifications
You must be signed in to change notification settings - Fork 31
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
fix: DH-17454: Wrap Modal in SpectrumThemeProvider #2169
fix: DH-17454: Wrap Modal in SpectrumThemeProvider #2169
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2169 +/- ##
==========================================
+ Coverage 46.59% 46.67% +0.07%
==========================================
Files 682 692 +10
Lines 38441 38628 +187
Branches 9578 9752 +174
==========================================
+ Hits 17912 18028 +116
- Misses 20519 20547 +28
- Partials 10 53 +43
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
||
background-color: var(--dh-spectrum-theme-provider-bg); | ||
color: var(--dh-color-text); | ||
display: contents; |
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.
seems like this would probably break something else, as that existing code was serving a purpose.
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've reverted the change as it wasn't needed after wrapping the Modal
in SpectrumThemeProvider
.
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.
Confirmed working.
I checked whether we need z-index for popper so spectrum date picker works there too, but it actually doesn't work at all because the spectrum datepicker popper closes the our popper. That'll be a separate issue if we ever decide to place a picker in a poper. Probably not an issue for dh.ui as we wouldn't ever expose our popper, only the spectrum version.
Wrap the
Modal
component attached todocument.body
inSpectrumThemeProvider
.Had to add
zIndex: 0
andposition: relative
to theSpectrumThemeProvider
to fix the issue withDatePicker
popovers nested in theModal
rendering behind it.