fix(sdk): missing css variables when rendering under a portal #44473
Conversation
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 105a72a...d79a67f.
|
|
| @font-face { | ||
| font-family: "Custom"; | ||
| src: url(${encodeURI(file.src)}) format("${file.fontFormat}"); | ||
| font-weight: ${file.fontWeight}; | ||
| font-style: normal; | ||
| font-display: swap; | ||
| } |
There was a problem hiding this comment.
@font-face declarations are always global in nature. It is not scoped. We don't need to add them to PublicComponentWrapper
| fontFiles: FontFile[] | null; | ||
| } | ||
| >` | ||
| ${({ theme }) => getMetabaseCssVariables(theme)} |
There was a problem hiding this comment.
We need this because of components like Popovers. Popovers still need the Metabase CSS variables for their colors, so we inject the CSS variables in both SdkGlobalStyles (applies to Mantine components) and PublicComponentStylesWrapper (applies to Metabase components). Refer to this thread for more info.
There was a problem hiding this comment.
This is what happens when we only inject the CSS variables in the component wrapper. It breaks quite a lot of places.
| * frontend/src/metabase/styled-components/containers/GlobalStyles/GlobalStyles.tsx | ||
| * enterprise/frontend/src/embedding-sdk/components/private/SdkContentWrapper.tsx | ||
| * frontend/src/metabase/styled-components/theme/css-variables.ts |
There was a problem hiding this comment.
I want to remove the CSS variables from GlobalStyles and rely on getMetabaseCssVariables, but that's more testing surface so let's do that in the next PR where I can focus on testing. If we do that, there will be a lot less duplication between GlobalStyles and SDK.
|
Note: I'll add visual test or unit test in this PR. Adding reviewers for now to see if this makes sense to other folks. |
…iables-when-under-portal
| width: 100%; | ||
| ${({ theme }) => getMetabaseCssVariables(theme)} | ||
|
|
||
| :where(svg) { |
There was a problem hiding this comment.
Overall I like the approach, but we need to guarantee that other global styles which are needed will be also copied here. I think we should add a common css block that contains such common styles
There was a problem hiding this comment.
@deniskaber good idea! i'll take a look at the CSS to see if there are more styles we can extract to here.
There was a problem hiding this comment.
@deniskaber I re-checked the CSS, and I've moved aceEditorStyles and saveDomImageStyles here, as well as adding color and font-family that matches GlobalStyles.
The only thing to be left in SdkGlobalStyles would then be font-size (cannot be in PublicComponentStyles due to cascades), rootStyle and font file loading (@font-face is global).
Closes #44391
Description
When SDK users are rendering the embedding SDK components (such as an interactive question) under a React portal (such as in a modal using their library of choice), the filter colors from the theme settings are not applied. In the screenshot below, you can notice that the filter color is not applied properly in (1).
This PR separates the components for injecting CSS variables to a public component (i.e.
PublicComponentStylesWrapper) from the components injecting global sdk styles such as font-face declarations (i.e.SdkGlobalStylesWrapper), so we can provide the CSS variables in context of React portals.How to verify
fix-react-portalbranch under the Shoppy demo appDemo
Checklist