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(toolbar): Fix tooltip & hovercard & menu react-portal rendering inside the toolbar #74797

Merged
merged 3 commits into from
Jul 24, 2024

Conversation

ryan953
Copy link
Member

@ryan953 ryan953 commented Jul 23, 2024

What you can barely see here is a css change in the before & after shots.

In the before there's no css applied. After there is some css (but z-index is too small, that's fixed in 37a40c6 in this PR)
What's happening?

We are importing a component that eventually calls, in the before case: createPortal(..., document.body). The trouble with that is where the import happens. In the website codebase we import it and the whole react tree, including emotion css, is inside of document.

But in this case we're importing it into toolbar code, so the react root starts inside our ShadowRoot reference instead of document.body. All our toolbar css is inside the shadowroot too, so when we use emotion and generate a className like sentry-devtools-b0afu4 nodes mounted at document won't be able to access it.

TL/DR: the portal target inside the toolbar needs to match with this snippet, using the same container value:

const myCache = useMemo(
() =>
createCache({
key: 'sentry-devtools',
stylisPlugins: [
/* your plugins here */
],
container,
prepend: false,
}),
[container]
);
, but only while we're developing inside sentry, and sharing components fully like this.

Before
SCR-20240723-nxfy

After
SCR-20240723-nxek

@ryan953 ryan953 requested a review from a team July 23, 2024 22:52
@github-actions github-actions bot added the Scope: Frontend Automatically applied to PRs that change frontend components label Jul 23, 2024
Copy link

codecov bot commented Jul 23, 2024

Bundle Report

Changes will increase total bundle size by 3.59kB ⬆️

Bundle name Size Change
app-webpack-bundle-array-push 28.18MB 3.59kB ⬆️

@ryan953 ryan953 merged commit 6be923a into master Jul 24, 2024
42 checks passed
@ryan953 ryan953 deleted the ryan953/toolbar-portal-styles branch July 24, 2024 22:04
Copy link

sentry-io bot commented Jul 24, 2024

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

  • ‼️ Error: Target container is not a DOM element. /replays/5892066ad3f24343bf8cce28d00f93ad View Issue
  • ‼️ Error: Target container is not a DOM element. /replays/5892066ad3f24343bf8cce28d00f93ad View Issue
  • ‼️ Error: Target container is not a DOM element. /replays/:replaySlug/ View Issue
  • ‼️ Error: Target container is not a DOM element. /replays/:replaySlug/ View Issue
  • ‼️ Error: Target container is not a DOM element. /replays/:replaySlug/ View Issue

Did you find this useful? React with a 👍 or 👎

@leeandher leeandher added the Trigger: Revert add to a merged PR to revert it (skips CI) label Jul 25, 2024
@getsentry-bot
Copy link
Contributor

PR reverted: ce3031a

getsentry-bot added a commit that referenced this pull request Jul 25, 2024
…dering inside the toolbar (#74797)"

This reverts commit 6be923a.

Co-authored-by: leeandher <35509934+leeandher@users.noreply.github.com>
Christinarlong pushed a commit that referenced this pull request Jul 26, 2024
…nside the toolbar (#74797)

What you can barely see here is a css change in the before & after
shots.

In the before there's no css applied. After there is some css (but
z-index is too small, that's fixed in
37a40c6
in this PR)
What's happening?

We are importing a component that eventually calls, in the before case:
`createPortal(..., document.body)`. The trouble with that is where the
import happens. In the website codebase we import it and the whole react
tree, including emotion css, is inside of `document`.

But in this case we're importing it into toolbar code, so the react root
starts inside our `ShadowRoot` reference instead of `document.body`. All
our toolbar css is inside the shadowroot too, so when we use emotion and
generate a className like `sentry-devtools-b0afu4` nodes mounted at
`document` won't be able to access it.

**TL/DR:** the portal target inside the toolbar needs to match with this
snippet, using the same `container` value:
https://github.com/getsentry/sentry/blob/6bb4638c3f24b9d737b80730561748874f4a7558/static/app/components/devtoolbar/components/providers.tsx#L23-L34,
but only while we're developing inside sentry, and sharing components
fully like this.

**Before**

![SCR-20240723-nxfy](https://github.com/user-attachments/assets/f2690b8b-3315-4e19-a838-b4f178bfcf0f)

**After**

![SCR-20240723-nxek](https://github.com/user-attachments/assets/80f4e159-cadb-4c1d-8479-933cb978275d)
Christinarlong pushed a commit that referenced this pull request Jul 26, 2024
…dering inside the toolbar (#74797)"

This reverts commit 6be923a.

Co-authored-by: leeandher <35509934+leeandher@users.noreply.github.com>
@github-actions github-actions bot locked and limited conversation to collaborators Aug 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Scope: Frontend Automatically applied to PRs that change frontend components Trigger: Revert add to a merged PR to revert it (skips CI)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants