Add proper brand colors to drills menu #42663

deniskaber · 2024-05-14T20:47:15Z

Closes #42560 and #42680

Description

This adds theming possibilities for InteractiveQuestion - filters, drills, table content.

How to verify

Describe the steps to verify that the changes are working as expected.

build SDK and use it with https://github.com/metabase/embedding-sdk-customer-zero/tree/42560-q-theming
change colors in https://github.com/metabase/embedding-sdk-customer-zero/blob/42560-q-theming/src/components/AppProvider.tsx#L33
Content should be updated according to the theme.

Demo

Screen.Recording.2024-05-16.at.02.51.02.mov

Checklist

Tests have been added/updated to cover changes in this PR

github-actions · 2024-05-14T20:48:28Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 752bf84...972134c.

Notify	File(s)
@alxnddr	frontend/src/metabase/visualizations/components/ClickActions/ClickActionControl.module.css frontend/src/metabase/visualizations/components/ClickActions/ClickActionControl.styled.tsx frontend/src/metabase/visualizations/components/ClickActions/ClickActionsPopover.styled.tsx frontend/src/metabase/visualizations/components/ClickActions/ClickActionsViewSection.styled.tsx frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.module.css frontend/src/metabase/visualizations/visualizations/PieChart/PieChart.module.css
@kdoh	frontend/src/metabase/ui/components/buttons/Button/Button.styled.tsx frontend/src/metabase/ui/components/inputs/Input/Input.styled.tsx frontend/src/metabase/ui/components/inputs/Select/Select.styled.tsx frontend/src/metabase/ui/components/overlays/HoverCard/HoverCard.styled.tsx frontend/src/metabase/ui/components/overlays/Menu/Menu.styled.tsx frontend/src/metabase/ui/components/overlays/Popover/Popover.styled.tsx frontend/src/metabase/ui/utils/colors.ts
@ranquild	enterprise/frontend/src/metabase-enterprise/whitelabel/lib/whitelabel.js

replay-io · 2024-05-14T20:59:38Z

Status	Complete ↗︎
Commit	`972134c`
Results	⚠️ 6 Flaky ✅ 2519 Passed

heypoom

Looks good to me so far! I think we should keep the external color scheme separate from the internal ones though, because of #42680 (explained in the below comments).

This is also because some of our colors might not make sense to external users, such as error and success used in smart scalar to indicate negative and positive changes (best to remap to negative and positive as discussed with Alessio), and we can provide public-facing TSDocs on those.

Also a note on the use of global mutations in useMemo, which might be nice if we can replace that with useEffect or other construct to better indicate we're doing impure mutations.

heypoom · 2024-05-16T10:03:07Z

enterprise/frontend/src/embedding-sdk/components/private/SdkContentWrapper.tsx

+  --mb-color-bg-white: ${({ theme }) => theme.fn.themeColor("bg-white")};
+  --mb-color-bg-black: ${({ theme }) => theme.fn.themeColor("bg-black")};


note to future self: we should consider remapping bg-black and bg-white when we are tackling #42680 to remap colors to make sense for dark mode users

heypoom · 2024-05-16T10:04:40Z

enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx

+    if (theme?.colors) {
+      Object.entries(theme.colors).forEach(([key, value]) => {
+        colors[key as keyof MetabaseColors] = value;
+      });
+    } else {
+      Object.entries(originalColors).forEach(([key, value]) => {
+        colors[key as keyof MetabaseColors] = value;
+      });
+    }


I wonder if we only pass in partial theme.colors, does the rest of the colors (i.e. originalColors) still gets applied? For instance, if I set text-dark: '#2d2d30' but didn't set anything else, does the default mb-brand-color gets applied? I'm asking this because of what the else branch is doing.

Also, I think we are globally mutating the colors object here, so semantically I don't think putting it in useMemo make sense, as useMemo usually communicates purity.

Also, I think we are globally mutating the colors object here, so semantically I don't think putting it in useMemo make sense, as useMemo usually communicates purity.

Semantically - yes, but we need updated colors object to calculate mantine theme overrides. I don't think we should enforce semantics and introduce extra rerenders because of it.

@deniskaber Yeah, I couldn't think of a way to cleanly do this without an extra re-render cycle either, so I think this will have to do 👍🏻

A way to make this logic jump out a bit more would be to move it to a named function, say setGlobalMetabaseColors. It kinda makes the logic jumps out more that it's not doing the usual thing that you would expect it to, so the reader knows to be prepared for that

heypoom · 2024-05-16T10:10:03Z

enterprise/frontend/src/embedding-sdk/types/theme/index.ts

-export interface MetabaseColors {
-  /** Primary brand color */
-  brand?: string;
-
-  /** Text color on dark elements. Should be a lighter color for readability. */
-  "text-dark"?: string;
-
-  /** Text color on light elements. Should be a darker color for readability. */
-  "text-light"?: string;
-
-  /** Lighter variation of dark text on light elements. */
-  "text-medium"?: string;
-}
-
-export type MetabaseColor = keyof MetabaseColors;
+export type MetabaseColors = ColorPalette;


@deniskaber we have a plan to remap the external-facing color names to the internal ones in #42680, as the names we use currently (e.g. text-dark and text-light) does not make sense for SDK customers who uses dark mode, so the idea there is to remap them (e.g. text-primary to text-dark, text-secondary to text-light and so on) - see the discussion here

If we don't remap the color names, then we would have to do a rename for the entire codebase - which is going to be a lot of diffs (and potentially cause confusion internally)

heypoom · 2024-05-16T10:11:57Z

enterprise/frontend/src/metabase-enterprise/whitelabel/lib/whitelabel.js

@@ -2,7 +2,7 @@ import { colors } from "metabase/lib/colors/palette";
 import MetabaseSettings from "metabase/lib/settings";

 export function updateColors() {
-  const scheme = MetabaseSettings.get("application-colors");
+  const scheme = MetabaseSettings.get("application-colors") || {};


Just a curiosity question, but I think this field is unset when the application colors haven't been overridden in the appearance settings? So, I think this value is only populated when the users set them in the admin panel? If so, using a default like this should work fine.

cc @WiNloSt we were discussing about this issue in Slack/GitHub earlier

Yeah, just retested. And also from the default value, it will always return a map. That means you don't need || {} part.

metabase/src/metabase/public_settings.clj

Lines 341 to 349 in 44cb385

(defsetting application-colors

(deferred-tru "Choose the colors used in the user interface throughout Metabase and others specifically for the charts. You need to refresh your browser to see your changes take effect.")

:visibility :public

:export? true

:type :json

:feature :whitelabel

:default {}

:audit :getter

:doc "To change the user interface colors:

@WiNloSt it was returning undefined or null in the SDK though, so I think that is why Denis added the || {} part. Doesn't that happen to you when running the demo app?

adding || {} looks like a hack to me if we expect MetabaseSettings.get('application-colors ') to always return a map.

By doing this wouldn't we just ignore the fact that application-colors doesn't return the correct value? That means when users set colors in appearance settings, if we don't provide theme would those colors even work?

That's what I want to know as well - why wasn't it returning a map despite public-settings.clj...

My (probably wrong) guess is that if application-colors is not set, then the default colors defined in colors would get applied. However, when one or more colors is set then it's going to return a map. That would explain this diff I saw above as well. Would be great if @deniskaber could confirm our assumptions here 🙏🏻

} else { Object.entries(originalColors).forEach(([key, value]) => { colors[key as keyof MetabaseColors] = value; }); }

Well I don't think the color from instance is working, my brand needs to be a distinct shade of blue (almost purple)

Also, this seems to make the admin settings look off. Can you confirm that too @heypoom ?

@WiNloSt yes, can confirm on my end that this breaks the dashboard styling. Thanks for running this locally!

For the customized user interface colors issue, let's tackle this separately in #42705.

Hey @heypoom , @WiNloSt . In the main metabase app settings get populated into MetabaseSettings object from server on page load - https://github.com/metabase/metabase/blob/master/resources/frontend_client/index_template.html#L27
So, when this piece of code gets executed, we already have MetabaseSettings object initialized with real values.

But in the SDK context we don't have this logic. MetabaseSettings is empty until we load user settings in provider, which is already late. We didn't have this issue in early builds because we haven't used EE plugins.
So, ideally we should initialize MetabaseSettings with some default settings, or load real settings before importing plugins (which is very tricky).

Here I added a fix that blocked me from running the demo. I can revert it if needed

@deniskaber I see, thanks for the explanation! As I will be working on #42705 next, do you know if there's a way we can retrieve the application-colors object (that was previously from {{bootstrapJSON}}) dynamically rather than relying on index_template.html? So I can update the updateColors logic accordingly.

I think it's OK if this PR doesn't cover the customized ui colors logic, I can tackle that in my PR later. I think we can stick with || {} for now just to not throw the error, no need to revert it.

We can focus on resolving the dashboard styling issue (blue background color leaking into buttons) in the above screenshot for now 🙏🏻

@heypoom , we get application-colors in /api/user/current api request which happens here - https://github.com/metabase/metabase/blob/master/enterprise/frontend/src/embedding-sdk/hooks/private/use-init-data.ts#L75
During SDK initialization basically (but after user auth).

heypoom

Some of the changes here appears to break the dashboard styling - see the screenshots here. Could you help to investigate this? Also added some review comments above 🙏🏻

# Conflicts: # enterprise/frontend/src/embedding-sdk/components/private/SdkContentWrapper.tsx # enterprise/frontend/src/embedding-sdk/lib/theme/get-embedding-theme.ts # frontend/src/metabase/visualizations/components/ClickActions/ClickActionControl.styled.tsx # frontend/src/metabase/visualizations/components/TableInteractive/TableInteractive.module.css

heypoom · 2024-05-17T10:39:37Z

Thanks! Re-testing this locally now

heypoom

I re-tested locally and the color leak issue in the dashboard no longer happens, looks great so far! Thanks for re-mapping the colors too 😄

heypoom · 2024-05-17T10:46:14Z

enterprise/frontend/src/embedding-sdk/components/public/MetabaseProvider.tsx

@@ -34,6 +38,12 @@ const MetabaseProviderInternal = ({
  theme,
 }: MetabaseProviderProps): JSX.Element => {
  const themeOverride = useMemo(() => {
+    const combinedThemeColors = getThemedColorsPallete(theme?.colors);


nit: typo

Suggested change

const combinedThemeColors = getThemedColorsPallete(theme?.colors);

const combinedThemeColors = getThemedColorsPalette(theme?.colors);

heypoom · 2024-05-17T10:46:52Z

enterprise/frontend/src/embedding-sdk/lib/theme/default-component-theme.ts

@@ -27,7 +27,7 @@ export const DEFAULT_EMBEDDED_COMPONENT_THEME: MetabaseComponentTheme = merge(
  {
    table: {
      cell: {
-        backgroundColor: "white",
+        backgroundColor: "bg-white",


Good catch - thanks!

heypoom · 2024-05-17T12:07:35Z

enterprise/frontend/src/embedding-sdk/lib/theme/get-embedding-theme.ts

+
+const originalColors = { ...colors };
+
+export function getThemedColorsPallete(


Suggested change

export function getThemedColorsPallete(

export function getThemedColorsPalette(

Add proper brand colors to drills menu

53830e6

deniskaber self-assigned this May 14, 2024

metabase-bot bot added .Team/Embedding visual Run Percy visual testing labels May 14, 2024

deniskaber added 3 commits May 16, 2024 02:36

Add theming for filters and drills

b49f2e8

Fix css vars

939e6fd

Fix table css

66c9e61

deniskaber requested a review from a team May 15, 2024 23:59

heypoom approved these changes May 16, 2024

View reviewed changes

heypoom mentioned this pull request May 16, 2024

Custom user interface colors does not get applied on SDK #42705

Closed

heypoom self-requested a review May 16, 2024 11:00

heypoom requested changes May 16, 2024

View reviewed changes

deniskaber added 2 commits May 16, 2024 18:01

Add SDK theme colors interface and remapping

3c3f620

Fix dashboard brand color issue

3101911

deniskaber requested review from WiNloSt, heypoom and a team May 16, 2024 15:18

deniskaber added 2 commits May 16, 2024 18:38

Fix theme colors interface

520759c

deniskaber added the embedding-sdk-build Triggers embedding SDK package build label May 16, 2024

Fix default theme table cell background

e9090ec

deniskaber added the no-backport Do not backport this PR to any branch label May 16, 2024

deniskaber mentioned this pull request May 16, 2024

feat(sdk): document theming options in readme #42784

Merged

heypoom approved these changes May 17, 2024

View reviewed changes

heypoom requested a review from a team May 17, 2024 12:09

deniskaber added 2 commits May 17, 2024 15:43

Fix typo

801c440

Merge branch 'master' into 42560-q-theming

58e64ee

Fix for qb filters panel custom border

972134c

deniskaber enabled auto-merge (squash) May 17, 2024 12:57

deniskaber merged commit a6035a1 into master May 17, 2024
108 checks passed

deniskaber deleted the 42560-q-theming branch May 17, 2024 13:40

This was referenced May 20, 2024

Override user interface colors in embedding SDK #42684

Closed

Background color settings for dashboard and visualization background in embedding SDK #42678

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add proper brand colors to drills menu #42663

Add proper brand colors to drills menu #42663

deniskaber commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024 •

edited

replay-io bot commented May 14, 2024 •

edited

heypoom left a comment •

edited

heypoom May 16, 2024

heypoom May 16, 2024 •

edited

deniskaber May 16, 2024

heypoom May 16, 2024

heypoom May 16, 2024 •

edited

heypoom May 16, 2024

heypoom May 16, 2024

WiNloSt May 16, 2024 •

edited

heypoom May 16, 2024 •

edited

WiNloSt May 16, 2024

heypoom May 16, 2024 •

edited

WiNloSt May 16, 2024 •

edited

heypoom May 16, 2024 •

edited

deniskaber May 16, 2024

heypoom May 16, 2024 •

edited

deniskaber May 16, 2024

heypoom left a comment •

edited

heypoom commented May 17, 2024

heypoom left a comment

heypoom May 17, 2024

heypoom May 17, 2024

heypoom May 17, 2024

		--mb-color-bg-white: ${({ theme }) => theme.fn.themeColor("bg-white")};
		--mb-color-bg-black: ${({ theme }) => theme.fn.themeColor("bg-black")};

	(defsetting application-colors
	(deferred-tru "Choose the colors used in the user interface throughout Metabase and others specifically for the charts. You need to refresh your browser to see your changes take effect.")
	:visibility :public
	:export? true
	:type :json
	:feature :whitelabel
	:default {}
	:audit :getter
	:doc "To change the user interface colors:

	const combinedThemeColors = getThemedColorsPallete(theme?.colors);
	const combinedThemeColors = getThemedColorsPalette(theme?.colors);


		const originalColors = { ...colors };

		export function getThemedColorsPallete(

	export function getThemedColorsPallete(
	export function getThemedColorsPalette(

Add proper brand colors to drills menu #42663

Add proper brand colors to drills menu #42663

Conversation

deniskaber commented May 14, 2024 • edited

Description

How to verify

Demo

Checklist

github-actions bot commented May 14, 2024 • edited

replay-io bot commented May 14, 2024 • edited

heypoom left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heypoom May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heypoom May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WiNloSt May 16, 2024 • edited

Choose a reason for hiding this comment

heypoom May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heypoom May 16, 2024 • edited

Choose a reason for hiding this comment

WiNloSt May 16, 2024 • edited

Choose a reason for hiding this comment

heypoom May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heypoom May 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heypoom left a comment • edited

Choose a reason for hiding this comment

heypoom commented May 17, 2024

heypoom left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deniskaber commented May 14, 2024 •

edited

github-actions bot commented May 14, 2024 •

edited

replay-io bot commented May 14, 2024 •

edited

heypoom left a comment •

edited

heypoom May 16, 2024 •

edited

heypoom May 16, 2024 •

edited

WiNloSt May 16, 2024 •

edited

heypoom May 16, 2024 •

edited

heypoom May 16, 2024 •

edited

WiNloSt May 16, 2024 •

edited

heypoom May 16, 2024 •

edited

heypoom May 16, 2024 •

edited

heypoom left a comment •

edited