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(sdk): missing css variables when rendering under a portal #44473

Merged
merged 9 commits into from
Jun 25, 2024
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { useSdkSelector } from "embedding-sdk/store";
import { getIsInitialized } from "embedding-sdk/store/selectors";
import type { SDKConfig } from "embedding-sdk/types";

import { SdkContentWrapper } from "./SdkContentWrapper";
import { SdkGlobalStylesWrapper } from "./SdkGlobalStylesWrapper";

interface AppInitializeControllerProps {
children: ReactNode;
Expand All @@ -25,11 +25,11 @@ export const AppInitializeController = ({
const isInitialized = useSdkSelector(getIsInitialized);

return (
<SdkContentWrapper
<SdkGlobalStylesWrapper
baseUrl={config.metabaseInstanceUrl}
id={EMBEDDING_SDK_ROOT_ELEMENT_ID}
>
{!isInitialized ? <div>{t`Loading…`}</div> : children}
</SdkContentWrapper>
</SdkGlobalStylesWrapper>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
import styled from "@emotion/styled";
import type { ReactNode } from "react";

import { color } from "metabase/lib/colors";
import { useThemeSpecificCssVariables } from "metabase/styled-components/theme/theme";

/**
* Injects CSS variables and styles to the SDK components underneath them.
* This is to ensure that the SDK components are styled correctly,
* even when rendered under a React portal.
*/
export function PublicComponentStylesWrapper({
children,
}: {
children: ReactNode;
}) {
const themeSpecificCssVariables = useThemeSpecificCssVariables();

return (
<StylesWrapperInner themeSpecificCssVariables={themeSpecificCssVariables}>
{children}
</StylesWrapperInner>
);
}

const StylesWrapperInner = styled.div<{
themeSpecificCssVariables: string;
}>`
/* NOTE: DO NOT ADD COLORS WITHOUT EXTREMELY GOOD REASON AND DESIGN REVIEW
* NOTE: KEEP SYNCHRONIZED WITH:
* frontend/src/metabase/ui/utils/colors.ts
* frontend/src/metabase/styled-components/containers/GlobalStyles/GlobalStyles.tsx
* frontend/src/metabase/css/core/colors.module.css
* .storybook/preview-head.html
*/
--mb-default-font-family: "${({ theme }) => theme.fontFamily}";
--mb-color-bg-light: ${({ theme }) => theme.fn.themeColor("bg-light")};
--mb-color-bg-dark: ${({ theme }) => theme.fn.themeColor("bg-dark")};
--mb-color-brand: ${({ theme }) => theme.fn.themeColor("brand")};

--mb-color-brand-light: color-mix(in srgb, var(--mb-color-brand) 53%, #fff);
--mb-color-brand-lighter: color-mix(in srgb, var(--mb-color-brand) 60%, #fff);
--mb-color-brand-alpha-04: color-mix(
in srgb,
var(--mb-color-brand) 4%,
transparent
);
--mb-color-brand-alpha-88: color-mix(
in srgb,
var(--mb-color-brand) 88%,
transparent
);

--mb-color-focus: ${({ theme }) => theme.fn.themeColor("focus")};
--mb-color-bg-white: ${({ theme }) => theme.fn.themeColor("bg-white")};
--mb-color-bg-black: ${({ theme }) => theme.fn.themeColor("bg-black")};
--mb-color-shadow: ${({ theme }) => theme.fn.themeColor("shadow")};
--mb-color-border: ${({ theme }) => theme.fn.themeColor("border")};
--mb-color-text-dark: ${({ theme }) => theme.fn.themeColor("text-dark")};
--mb-color-text-medium: ${({ theme }) => theme.fn.themeColor("text-medium")};
--mb-color-text-light: ${({ theme }) => theme.fn.themeColor("text-light")};
--mb-color-danger: ${({ theme }) => theme.fn.themeColor("danger")};
--mb-color-error: ${({ theme }) => theme.fn.themeColor("error")};
--mb-color-filter: ${({ theme }) => theme.fn.themeColor("filter")};
--mb-color-bg-error: ${() => color("bg-error")};
--mb-color-bg-medium: ${({ theme }) => theme.fn.themeColor("bg-medium")};
--mb-color-bg-night: ${() => color("bg-night")};
--mb-color-text-white: ${({ theme }) => theme.fn.themeColor("text-white")};
--mb-color-success: ${({ theme }) => theme.fn.themeColor("success")};
--mb-color-summarize: ${({ theme }) => theme.fn.themeColor("summarize")};
--mb-color-warning: ${({ theme }) => theme.fn.themeColor("warning")};

/**
Theming-specific CSS variables.
Keep in sync with [GlobalStyles.tsx] and [.storybook/preview-head.html].

Refer to DEFAULT_METABASE_COMPONENT_THEME for their defaults.

These CSS variables are not part of the core design system colors.
Do NOT add them to [palette.ts] and [colors.ts].
*/
${({ themeSpecificCssVariables }) => themeSpecificCssVariables}

:where(svg) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@deniskaber good idea! i'll take a look at the CSS to see if there are more styles we can extract to here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

display: inline;
}
`;
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import type { JSX } from "react";
import { t } from "ttag";

import { PublicComponentStylesWrapper } from "embedding-sdk/components/private/PublicComponentStylesWrapper";
import { SdkError } from "embedding-sdk/components/private/PublicComponentWrapper/SdkError";
import { SdkLoader } from "embedding-sdk/components/private/PublicComponentWrapper/SdkLoader";
import { useSdkSelector } from "embedding-sdk/store";
Expand Down Expand Up @@ -29,5 +30,7 @@ export const PublicComponentWrapper = ({
return <SdkError message={loginStatus.error.message} />;
}

return children;
return (
<PublicComponentStylesWrapper>{children}</PublicComponentStylesWrapper>
);
};

This file was deleted.

Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { css } from "@emotion/react";
import styled from "@emotion/styled";
import type { HTMLAttributes } from "react";

import { rootStyle } from "metabase/css/core/base.styled";
import { defaultFontFiles } from "metabase/css/core/fonts.styled";
import { useSelector } from "metabase/lib/redux";
import { aceEditorStyles } from "metabase/query_builder/components/NativeQueryEditor/NativeQueryEditor.styled";
import { getFontFiles } from "metabase/styled-components/selectors";
import { saveDomImageStyles } from "metabase/visualizations/lib/save-chart-image";
import type { FontFile } from "metabase-types/api";

interface SdkContentWrapperProps {
baseUrl?: string;
}

export function SdkGlobalStylesWrapper({
baseUrl,
...divProps
}: SdkContentWrapperProps & HTMLAttributes<HTMLDivElement>) {
const fontFiles = useSelector(getFontFiles);

return (
<SdkGlobalStylesInner
baseUrl={baseUrl}
fontFiles={fontFiles}
{...divProps}
/>
);
}

const SdkGlobalStylesInner = styled.div<
SdkContentWrapperProps & {
fontFiles: FontFile[] | null;
}
>`
font-size: ${({ theme }) => theme.other.fontSize};

${aceEditorStyles}
${saveDomImageStyles}
${rootStyle}

${({ baseUrl }) => defaultFontFiles({ baseUrl })}

${({ fontFiles }) =>
fontFiles?.map(
file => css`
@font-face {
font-family: "Custom";
src: url(${encodeURI(file.src)}) format("${file.fontFormat}");
font-weight: ${file.fontWeight};
font-style: normal;
font-display: swap;
}
Comment on lines +47 to +53
Copy link
Contributor Author

@heypoom heypoom Jun 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@font-face declarations are always global in nature. It is not scoped. We don't need to add them to PublicComponentWrapper

`,
)}
`;
2 changes: 1 addition & 1 deletion frontend/src/metabase/css/core/colors.module.css
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* NOTE: KEEP SYNCRONIZED WITH:
* frontend/src/metabase/ui/utils/colors.ts
* frontend/src/metabase/styled-components/containers/GlobalStyles/GlobalStyles.tsx
* enterprise/frontend/src/embedding-sdk/components/private/SdkContentWrapper.tsx
* enterprise/frontend/src/embedding-sdk/components/private/PublicComponentStylesWrapper.tsx
* .storybook/preview-head.html
*/
:root {
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/metabase/lib/colors/palette.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ export const ACCENT_COUNT = 8;
// NOTE: KEEP SYNCRONIZED WITH:
// frontend/src/metabase/css/core/colors.module.css
// frontend/src/metabase/styled-components/containers/GlobalStyles/GlobalStyles.tsx
// enterprise/frontend/src/embedding-sdk/components/private/SdkContentWrapper.tsx
// enterprise/frontend/src/embedding-sdk/components/private/PublicComponentStylesWrapper.tsx
// .storybook/preview-head.html
export const colors = {
brand: "#509EE3",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { alpha, color, lighten } from "metabase/lib/colors";
import { getSitePath } from "metabase/lib/dom";
import { useSelector } from "metabase/lib/redux";
import { aceEditorStyles } from "metabase/query_builder/components/NativeQueryEditor/NativeQueryEditor.styled";
import { useThemeSpecificSelectors } from "metabase/styled-components/theme/theme";
import { useThemeSpecificCssVariables } from "metabase/styled-components/theme/theme";
import { saveDomImageStyles } from "metabase/visualizations/lib/save-chart-image";

import { getFont, getFontFiles } from "../../selectors";
Expand All @@ -15,7 +15,7 @@ export const GlobalStyles = (): JSX.Element => {
const font = useSelector(getFont);
const fontFiles = useSelector(getFontFiles);

const themeSpecificSelectors = useThemeSpecificSelectors();
const themeSpecificSelectors = useThemeSpecificCssVariables();

const sitePath = getSitePath();

Expand Down
5 changes: 2 additions & 3 deletions frontend/src/metabase/styled-components/theme/theme.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ type MetabaseComponentThemeKey = FlattenObjectKeys<MetabaseComponentTheme>;
Theming-specific CSS variables.
These CSS variables are not part of the core design system colors.
**/
export const useThemeSpecificSelectors = () => {
export const useThemeSpecificCssVariables = () => {
const theme = useMantineTheme();

// get value from theme.other, which is typed as MetabaseComponentTheme
// Get value from theme.other, which is typed as MetabaseComponentTheme
const getValue = (key: MetabaseComponentThemeKey) => get(theme.other, key);

return `
Expand All @@ -42,6 +42,5 @@ export const useThemeSpecificSelectors = () => {
--mb-color-bg-collection-browser-expand-button-hover: ${getValue(
"collectionBrowser.breadcrumbs.expandButton.hoverBackgroundColor",
)};

`;
};
Loading