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

fix: DH-17454: Wrap Modal in SpectrumThemeProvider #2169

Merged
merged 3 commits into from
Jul 25, 2024

Conversation

vbabich
Copy link
Collaborator

@vbabich vbabich commented Jul 23, 2024

Wrap the Modal component attached to document.body in SpectrumThemeProvider.

Had to add zIndex: 0 and position: relative to the SpectrumThemeProvider to fix the issue with DatePicker popovers nested in the Modal rendering behind it.

Screenshot 2024-07-23 at 4 35 35 PM

@vbabich vbabich self-assigned this Jul 23, 2024
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.67%. Comparing base (989b8ec) to head (bce2e25).
Report is 21 commits behind head on main.

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     
Flag Coverage Δ
unit 46.67% <ø> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vbabich vbabich changed the title fix: Ignore Theme Provider layout box fix: DH-17454: Ignore Theme Provider layout box Jul 23, 2024

background-color: var(--dh-spectrum-theme-provider-bg);
color: var(--dh-color-text);
display: contents;
Copy link
Contributor

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.

Copy link
Collaborator Author

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.

@vbabich vbabich marked this pull request as ready for review July 23, 2024 22:39
@vbabich vbabich requested a review from dsmmcken July 23, 2024 22:39
@vbabich vbabich changed the title fix: DH-17454: Ignore Theme Provider layout box fix: DH-17454: Wrap Modal in SpectrumThemeProvider Jul 23, 2024
Copy link
Contributor

@dsmmcken dsmmcken left a 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.

@vbabich vbabich merged commit 0058b18 into deephaven:main Jul 25, 2024
13 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants