From ed24366ea9855cb4e5f41eb76eef06d80e4ba21f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Nicol=C3=B2=20Pretto?= <info@npretto.com>
Date: Thu, 26 Sep 2024 18:53:00 +0200
Subject: [PATCH] fix(sdk): remove ts limitation on custom fonts + some minimal
e2e tests for the fonts (#48071)
* IS_EMBEDDING_SDK_BUILD -> IS_EMBEDDING_SDK to keep things simple, as we already have that name in other places
* don't load metabase css in sdk stories
* first two stories
* make the store not a global "singleton" anymore
* basic test for font-family styles in the SDK
* allow any custom font in theme.fontFamily as we actually support it
* fix tests using hardcoded default font family
---
.storybook/main.js | 2 +-
.storybook/preview.tsx | 36 ++--
e2e/support/helpers/e2e-embedding-helpers.js | 6 +
.../metabase-sdk-styles-tests.cy.spec.ts | 170 ++++++++++++++++++
.../components/private/SdkThemeProvider.tsx | 5 +-
.../components/public/MetabaseProvider.tsx | 11 +-
.../frontend/src/embedding-sdk/store/index.ts | 19 +-
.../test/CommonSdkStoryWrapper.tsx | 7 +-
.../tests/styling-sdk-tests.stories.tsx | 71 ++++++++
.../src/embedding-sdk/types/theme/index.ts | 6 +-
frontend/src/metabase/env.ts | 2 +-
webpack.embedding-sdk.config.js | 2 +-
12 files changed, 300 insertions(+), 37 deletions(-)
create mode 100644 e2e/test/scenarios/embedding-sdk/metabase-sdk-styles-tests.cy.spec.ts
create mode 100644 enterprise/frontend/src/embedding-sdk/tests/styling-sdk-tests.stories.tsx
diff --git a/.storybook/main.js b/.storybook/main.js
index a522fb6c9c8..3342d8e9867 100644
--- a/.storybook/main.js
+++ b/.storybook/main.js
@@ -55,7 +55,7 @@ module.exports = {
}),
new webpack.EnvironmentPlugin({
EMBEDDING_SDK_VERSION,
- IS_EMBEDDING_SDK_BUILD: isEmbeddingSDK,
+ IS_EMBEDDING_SDK: isEmbeddingSDK,
}),
],
module: {
diff --git a/.storybook/preview.tsx b/.storybook/preview.tsx
index cd48e8781ad..52a0d45125d 100644
--- a/.storybook/preview.tsx
+++ b/.storybook/preview.tsx
@@ -1,10 +1,14 @@
import React from "react";
import { ThemeProvider } from "metabase/ui";
-import "metabase/css/core/index.css";
-import "metabase/css/vendor.css";
-import "metabase/css/index.module.css";
-import "metabase/lib/dayjs";
+const isEmbeddingSDK = process.env.IS_EMBEDDING_SDK === "true";
+
+if (!isEmbeddingSDK) {
+ require("metabase/css/core/index.css");
+ require("metabase/css/vendor.css");
+ require("metabase/css/index.module.css");
+ require("metabase/lib/dayjs");
+}
import { EmotionCacheProvider } from "metabase/styled-components/components/EmotionCacheProvider";
import { getMetabaseCssVariables } from "metabase/styled-components/theme/css-variables";
@@ -35,17 +39,19 @@ const globalStyles = css`
${baseStyle}
`;
-export const decorators = [
- renderStory => (
- <EmotionCacheProvider>
- <ThemeProvider>
- <Global styles={globalStyles} />
- <CssVariables />
- {renderStory()}
- </ThemeProvider>
- </EmotionCacheProvider>
- ),
-];
+export const decorators = isEmbeddingSDK
+ ? [] // No decorators for Embedding SDK stories, as we want to simulate real use cases
+ : [
+ renderStory => (
+ <EmotionCacheProvider>
+ <ThemeProvider>
+ <Global styles={globalStyles} />
+ <CssVariables />
+ {renderStory()}
+ </ThemeProvider>
+ </EmotionCacheProvider>
+ ),
+ ];
function CssVariables() {
const theme = useTheme();
diff --git a/e2e/support/helpers/e2e-embedding-helpers.js b/e2e/support/helpers/e2e-embedding-helpers.js
index bcfb8533411..c6f03faf715 100644
--- a/e2e/support/helpers/e2e-embedding-helpers.js
+++ b/e2e/support/helpers/e2e-embedding-helpers.js
@@ -262,6 +262,12 @@ export function createPublicDashboardLink(dashboardId) {
return cy.request("POST", `/api/dashboard/${dashboardId}/public_link`, {});
}
+/**
+ * @param {Object} options
+ * @param {string} options.url
+ * @param {Object} options.qs
+ * @param {Function} [options.onBeforeLoad]
+ */
export const visitFullAppEmbeddingUrl = ({ url, qs, onBeforeLoad }) => {
cy.visit({
url,
diff --git a/e2e/test/scenarios/embedding-sdk/metabase-sdk-styles-tests.cy.spec.ts b/e2e/test/scenarios/embedding-sdk/metabase-sdk-styles-tests.cy.spec.ts
new file mode 100644
index 00000000000..f3b05af7126
--- /dev/null
+++ b/e2e/test/scenarios/embedding-sdk/metabase-sdk-styles-tests.cy.spec.ts
@@ -0,0 +1,170 @@
+/* eslint-disable no-unscoped-text-selectors */
+import { ORDERS_QUESTION_ID } from "e2e/support/cypress_sample_instance_data";
+import {
+ restore,
+ setTokenFeatures,
+ visitFullAppEmbeddingUrl,
+} from "e2e/support/helpers";
+import {
+ EMBEDDING_SDK_STORY_HOST,
+ describeSDK,
+} from "e2e/support/helpers/e2e-embedding-sdk-helpers";
+import {
+ JWT_SHARED_SECRET,
+ setupJwt,
+} from "e2e/support/helpers/e2e-jwt-helpers";
+
+const STORIES = {
+ NO_STYLES_SUCCESS: "embeddingsdk-styles-tests--no-styles-success",
+ NO_STYLES_ERROR: "embeddingsdk-styles-tests--no-styles-error",
+ FONT_FROM_CONFIG: "embeddingsdk-styles-tests--font-from-config",
+ GET_BROWSER_DEFAUL_FONT:
+ "embeddingsdk-styles-tests--get-browser-default-font",
+} as const;
+
+describeSDK("scenarios > embedding-sdk > static-dashboard", () => {
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+ setTokenFeatures("all");
+ setupJwt();
+ cy.signOut();
+ });
+
+ const wrapBrowserDefaultFont = () => {
+ visitFullAppEmbeddingUrl({
+ url: EMBEDDING_SDK_STORY_HOST,
+ qs: {
+ id: STORIES.GET_BROWSER_DEFAUL_FONT,
+ viewMode: "story",
+ },
+ });
+
+ cy.findByText("paragraph with default browser font").then($element => {
+ const fontFamily = $element.css("font-family");
+ cy.wrap(fontFamily).as("defaultBrowserFonteFamily");
+ });
+ };
+
+ describe("style leaking", () => {
+ it("[success scenario] should use the default fonts outside of our components, and Lato on our components", () => {
+ wrapBrowserDefaultFont();
+
+ visitFullAppEmbeddingUrl({
+ url: EMBEDDING_SDK_STORY_HOST,
+ qs: {
+ id: STORIES.NO_STYLES_SUCCESS,
+ viewMode: "story",
+ },
+ onBeforeLoad: (window: any) => {
+ window.JWT_SHARED_SECRET = JWT_SHARED_SECRET;
+ window.METABASE_INSTANCE_URL = Cypress.config().baseUrl;
+ window.QUESTION_ID = ORDERS_QUESTION_ID;
+ },
+ });
+
+ cy.get("@defaultBrowserFonteFamily").then(defaultBrowserFonteFamily => {
+ cy.findByText("This is outside of the provider").should(
+ "have.css",
+ "font-family",
+ defaultBrowserFonteFamily,
+ );
+ cy.findByText("This is inside of the provider").should(
+ "have.css",
+ "font-family",
+ defaultBrowserFonteFamily,
+ );
+ cy.findByText("Product ID").should(
+ "have.css",
+ "font-family",
+ "Lato, sans-serif",
+ );
+ });
+ });
+
+ it("[error scenario] should use the default fonts outside of our components, and Lato on our components", () => {
+ wrapBrowserDefaultFont();
+
+ visitFullAppEmbeddingUrl({
+ url: EMBEDDING_SDK_STORY_HOST,
+ qs: {
+ id: STORIES.NO_STYLES_ERROR,
+ viewMode: "story",
+ },
+ onBeforeLoad: (window: any) => {
+ window.JWT_SHARED_SECRET = JWT_SHARED_SECRET;
+ window.METABASE_INSTANCE_URL = Cypress.config().baseUrl;
+ window.QUESTION_ID = ORDERS_QUESTION_ID;
+ },
+ });
+
+ cy.get("@defaultBrowserFonteFamily").then(defaultBrowserFonteFamily => {
+ cy.findByText("This is outside of the provider").should(
+ "have.css",
+ "font-family",
+ defaultBrowserFonteFamily,
+ );
+
+ cy.findByText("This is inside of the provider").should(
+ "have.css",
+ "font-family",
+ defaultBrowserFonteFamily,
+ );
+
+ cy.findByText(
+ "Could not authenticate: invalid JWT URI or JWT provider did not return a valid JWT token",
+ ).should("have.css", "font-family", "Lato, sans-serif");
+ });
+ });
+ });
+
+ describe("fontFamily", () => {
+ it("should use the font from the theme if set", () => {
+ visitFullAppEmbeddingUrl({
+ url: EMBEDDING_SDK_STORY_HOST,
+ qs: {
+ id: STORIES.FONT_FROM_CONFIG,
+ viewMode: "story",
+ },
+ onBeforeLoad: (window: any) => {
+ window.JWT_SHARED_SECRET = JWT_SHARED_SECRET;
+ window.METABASE_INSTANCE_URL = Cypress.config().baseUrl;
+ window.QUESTION_ID = ORDERS_QUESTION_ID;
+ },
+ });
+
+ cy.findByText("Product ID").should(
+ "have.css",
+ "font-family",
+ "Impact, sans-serif",
+ );
+ });
+
+ it("should fallback to the font from the instance if no fontFamily is set on the theme", () => {
+ cy.signInAsAdmin();
+ cy.request("PUT", "/api/setting/application-font", {
+ value: "Roboto Mono",
+ });
+ cy.signOut();
+
+ visitFullAppEmbeddingUrl({
+ url: EMBEDDING_SDK_STORY_HOST,
+ qs: {
+ id: STORIES.NO_STYLES_SUCCESS,
+ viewMode: "story",
+ },
+ onBeforeLoad: (window: any) => {
+ window.JWT_SHARED_SECRET = JWT_SHARED_SECRET;
+ window.METABASE_INSTANCE_URL = Cypress.config().baseUrl;
+ window.QUESTION_ID = ORDERS_QUESTION_ID;
+ },
+ });
+
+ cy.findByText("Product ID").should(
+ "have.css",
+ "font-family",
+ '"Roboto Mono", sans-serif',
+ );
+ });
+ });
+});
diff --git a/enterprise/frontend/src/embedding-sdk/components/private/SdkThemeProvider.tsx b/enterprise/frontend/src/embedding-sdk/components/private/SdkThemeProvider.tsx
index 3e6c9d78b19..f29721404d4 100644
--- a/enterprise/frontend/src/embedding-sdk/components/private/SdkThemeProvider.tsx
+++ b/enterprise/frontend/src/embedding-sdk/components/private/SdkThemeProvider.tsx
@@ -2,6 +2,7 @@ import { Global } from "@emotion/react";
import { useMemo } from "react";
import type { MetabaseTheme } from "embedding-sdk";
+import { DEFAULT_FONT } from "embedding-sdk/config";
import {
getEmbeddingThemeOverride,
setGlobalEmbeddingColors,
@@ -42,7 +43,9 @@ export const SdkThemeProvider = ({ theme, children }: Props) => {
function GlobalSdkCssVariables() {
const theme = useMantineTheme();
- const font = useSelector(getFont);
+
+ // the default is needed for when the sdk can't connect to the instance and get the default from there
+ const font = useSelector(getFont) ?? DEFAULT_FONT;
return <Global styles={getMetabaseSdkCssVariables(theme, font)} />;
}
diff --git a/enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx b/enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx
index 4fbbf6bf3cc..c920e95614d 100644
--- a/enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx
+++ b/enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx
@@ -10,7 +10,7 @@ import {
import { useInitData } from "embedding-sdk/hooks";
import type { SdkEventHandlersConfig } from "embedding-sdk/lib/events";
import type { SdkPluginsConfig } from "embedding-sdk/lib/plugins";
-import { store } from "embedding-sdk/store";
+import { getSdkStore } from "embedding-sdk/store";
import {
setErrorComponent,
setEventHandlers,
@@ -95,9 +95,12 @@ export const MetabaseProviderInternal = ({
);
};
-export const MetabaseProvider = memo(function MetabaseProvider(
- props: MetabaseProviderProps,
-) {
+export const MetabaseProvider = memo(function MetabaseProvider({
+ // @ts-expect-error -- we don't want to expose the store prop
+ // eslint-disable-next-line react/prop-types
+ store = getSdkStore(),
+ ...props
+}: MetabaseProviderProps) {
return (
<Provider store={store}>
<MetabaseProviderInternal store={store} {...props} />
diff --git a/enterprise/frontend/src/embedding-sdk/store/index.ts b/enterprise/frontend/src/embedding-sdk/store/index.ts
index 5be403a894f..9571b6fc008 100644
--- a/enterprise/frontend/src/embedding-sdk/store/index.ts
+++ b/enterprise/frontend/src/embedding-sdk/store/index.ts
@@ -18,15 +18,16 @@ export const sdkReducers = {
sdk,
} as unknown as Record<string, Reducer>;
-export const store = getStore(sdkReducers, null, {
- embed: {
- options: {},
- isEmbeddingSdk: true,
- },
- app: {
- isDndAvailable: false,
- },
-}) as unknown as Store<SdkStoreState, AnyAction>;
+export const getSdkStore = () =>
+ getStore(sdkReducers, null, {
+ embed: {
+ options: {},
+ isEmbeddingSdk: true,
+ },
+ app: {
+ isDndAvailable: false,
+ },
+ }) as unknown as Store<SdkStoreState, AnyAction>;
export const useSdkDispatch: () => ThunkDispatch<
SdkStoreState,
diff --git a/enterprise/frontend/src/embedding-sdk/test/CommonSdkStoryWrapper.tsx b/enterprise/frontend/src/embedding-sdk/test/CommonSdkStoryWrapper.tsx
index fb1c056d246..13c9448b29d 100644
--- a/enterprise/frontend/src/embedding-sdk/test/CommonSdkStoryWrapper.tsx
+++ b/enterprise/frontend/src/embedding-sdk/test/CommonSdkStoryWrapper.tsx
@@ -11,7 +11,10 @@ const METABASE_JWT_SHARED_SECRET =
const secret = new TextEncoder().encode(METABASE_JWT_SHARED_SECRET);
-const DEFAULT_CONFIG: SDKConfig = {
+/**
+ * SDK config that signs the jwt on the FE
+ */
+export const storybookSdkDefaultConfig: SDKConfig = {
metabaseInstanceUrl: METABASE_INSTANCE_URL,
jwtProviderUri: `${METABASE_INSTANCE_URL}/sso/metabase`,
fetchRequestToken: async () => {
@@ -39,7 +42,7 @@ const DEFAULT_CONFIG: SDKConfig = {
};
export const CommonSdkStoryWrapper = (Story: Story) => (
- <MetabaseProvider config={DEFAULT_CONFIG}>
+ <MetabaseProvider config={storybookSdkDefaultConfig}>
<Story />
</MetabaseProvider>
);
diff --git a/enterprise/frontend/src/embedding-sdk/tests/styling-sdk-tests.stories.tsx b/enterprise/frontend/src/embedding-sdk/tests/styling-sdk-tests.stories.tsx
new file mode 100644
index 00000000000..ef9b445fcd7
--- /dev/null
+++ b/enterprise/frontend/src/embedding-sdk/tests/styling-sdk-tests.stories.tsx
@@ -0,0 +1,71 @@
+import {
+ MetabaseProvider,
+ StaticQuestion,
+} from "embedding-sdk/components/public";
+import { storybookSdkDefaultConfig } from "embedding-sdk/test/CommonSdkStoryWrapper";
+import type { SDKConfig } from "embedding-sdk/types";
+
+export default {
+ title: "EmbeddingSDK/styles tests",
+};
+
+const configThatWillError: SDKConfig = {
+ apiKey: "TEST",
+ metabaseInstanceUrl: "http://localhost",
+};
+
+/**
+ * This simulates an empty project with just the provider, we should not mess
+ * with the styles either inside or outside of the provider
+ */
+export const NoStylesError = () => (
+ <div>
+ <h1>No styles applied anywhere, should use browser default</h1>
+ <div style={{ border: "1px solid black" }}>
+ <h1>This is outside of the provider</h1>
+ </div>
+
+ <MetabaseProvider config={configThatWillError}>
+ <div style={{ border: "1px solid black" }}>
+ <h1>This is inside of the provider</h1>
+ </div>
+
+ <StaticQuestion questionId={(window as any).QUESTION_ID || 1} />
+ </MetabaseProvider>
+ </div>
+);
+
+export const NoStylesSuccess = () => (
+ <div>
+ <h1>No styles applied anywhere, should use browser default</h1>
+ <div style={{ border: "1px solid black" }}>
+ <h1>This is outside of the provider</h1>
+ </div>
+
+ <MetabaseProvider config={storybookSdkDefaultConfig}>
+ <div style={{ border: "1px solid black" }}>
+ <h1>This is inside of the provider</h1>
+ </div>
+
+ <StaticQuestion questionId={(window as any).QUESTION_ID || 1} />
+ </MetabaseProvider>
+ </div>
+);
+
+export const FontFromConfig = () => (
+ <div>
+ <MetabaseProvider
+ config={storybookSdkDefaultConfig}
+ theme={{ fontFamily: "Impact" }}
+ >
+ <StaticQuestion questionId={(window as any).QUESTION_ID || 1} />
+ </MetabaseProvider>
+ </div>
+);
+
+/**
+ * This story is only needed to get the default font of the browser
+ */
+export const GetBrowserDefaultFont = () => (
+ <p>paragraph with default browser font</p>
+);
diff --git a/enterprise/frontend/src/embedding-sdk/types/theme/index.ts b/enterprise/frontend/src/embedding-sdk/types/theme/index.ts
index 02473658763..26e84d44357 100644
--- a/enterprise/frontend/src/embedding-sdk/types/theme/index.ts
+++ b/enterprise/frontend/src/embedding-sdk/types/theme/index.ts
@@ -17,10 +17,10 @@ export interface MetabaseTheme {
fontSize?: string;
/**
- * Base font family supported by Metabase, defaults to `Lato`.
- * Custom fonts are not yet supported in this version.
+ * Font family that will be used for all text, it defaults to the instance's default font.
**/
- fontFamily?: MetabaseFontFamily;
+ // eslint-disable-next-line @typescript-eslint/ban-types -- this is needed to allow any string but keep autocomplete for the built-in ones
+ fontFamily?: MetabaseFontFamily | (string & {});
/** Base line height */
lineHeight?: string | number;
diff --git a/frontend/src/metabase/env.ts b/frontend/src/metabase/env.ts
index 5ad8833c1ca..40ac3d501db 100644
--- a/frontend/src/metabase/env.ts
+++ b/frontend/src/metabase/env.ts
@@ -12,4 +12,4 @@ export const shouldLogAnalytics = process.env.MB_LOG_ANALYTICS === "true";
export const isChartsDebugLoggingEnabled =
process.env.MB_LOG_CHARTS_DEBUG === "true";
-export const isEmbeddingSdk = !!process.env.IS_EMBEDDING_SDK_BUILD;
+export const isEmbeddingSdk = !!process.env.IS_EMBEDDING_SDK;
diff --git a/webpack.embedding-sdk.config.js b/webpack.embedding-sdk.config.js
index 1ff7056f86e..fcd7acc5244 100644
--- a/webpack.embedding-sdk.config.js
+++ b/webpack.embedding-sdk.config.js
@@ -149,7 +149,7 @@ module.exports = env => {
}),
new webpack.EnvironmentPlugin({
EMBEDDING_SDK_VERSION,
- IS_EMBEDDING_SDK_BUILD: true,
+ IS_EMBEDDING_SDK: true,
}),
new ForkTsCheckerWebpackPlugin({
async: isDevMode,
--
GitLab