create ui for the full embedding homepage, without any logic #40251

npretto · 2024-03-18T17:10:12Z

Part of [Epic] Initial homepage experience for embedding admins

Description

This only adds the "dumb" UI, all the logic and tests will come in separate PRs

Note that I'm currently importing List from mantine, I'll open a PR to re-export it from metabase/ui (see slack) and to also fix the numbers not showing up without the inline style.

How to verify

yarn storybook
open http://localhost:6006/?path=/story/features-embedhomepage--default
play around with the controls

npretto · 2024-03-19T13:41:45Z

frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.stories.tsx

@@ -0,0 +1,47 @@
+import type { Meta, StoryObj } from "@storybook/react";


This may or may not land in master (slack), but for now it's helpful to test all the variants of the UI.

I have no objection for having this in the repo if it helps with feature development.

npretto · 2024-03-19T13:47:12Z

frontend/src/metabase/home/components/EmbedHomepage/StaticTabContent.tsx

@@ -0,0 +1,65 @@
+// eslint-disable-next-line no-restricted-imports
+import { List } from "@mantine/core";


I'll move this to metabase/ui, see description of PR

npretto · 2024-03-19T13:47:38Z

frontend/src/metabase/home/components/EmbedHomepage/StaticTabContent.tsx

+        type="ordered"
+        mb="lg"
+        size="sm"
+        // TODO: fix this in a better way


I'll also fix this (I hope there's a easy way with the mantine overrides) when re-exporting List from metabase/ui

github-actions · 2024-03-19T13:52:23Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6710930...25fb1b4.

Notify	File(s)
@ranquild	frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.stories.tsx frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.tsx frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepageView.tsx frontend/src/metabase/home/components/EmbedHomepage/InteractiveTabContent.tsx frontend/src/metabase/home/components/EmbedHomepage/StaticTabContent.tsx frontend/src/metabase/home/components/EmbedHomepage/index.ts

replay-io · 2024-03-19T14:02:36Z

Status	Complete ↗︎
Commit	`25fb1b4`
Results	⚠️ 3 Flaky ✅ 2367 Passed

WiNloSt

Looks good with some minor comments.

WiNloSt · 2024-03-20T09:54:27Z

frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.tsx

+
+export const EmbedHomepage = () => {
+  const interactiveEmbeddingQuickStartUrl = useSelector(state =>
+    // eslint-disable-next-line no-unconditional-metabase-links-render -- only visible to admins


How do you feel about having to apply this everywhere? I feel like I erred on the side of being more cautious than being more relaxed since this is only inconvenient for us, but failing to hide user-facing links could make Metabase looks less polished.

If we allow things like disabling the rule for the whole file, it could be possible there are both links that are visible to end users and admins in the same file, though it might never happen.

And I think we only have to do this a handful of times per feature. There are less than 10 inline comments if I count this correctly.

Any feedback would be welcome.

it could be possible there are both links that are visible to end users and admins in the same file, though it might never happen.

Yeah that was my thought to, it may happen, maybe not, but it could. Adding the disable-next-line is a relatively low effort as the editor will do it for me so I'm fine with it

WiNloSt · 2024-03-20T09:56:53Z

frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.tsx

+      interactiveEmbeddingQuickstartUrl={interactiveEmbeddingQuickStartUrl}
+      embeddingDocsUrl={embeddingDocsUrl}
+      // eslint-disable-next-line no-unconditional-metabase-links-render -- only visible to admins
+      customerFacingAnalyticsDocsUrl="https://www.metabase.com/learn/customer-facing-analytics/"


The name feels a bit off. The comment suggests this link is only for admin, but the name suggests it's a user-facing string which, in theory, should be rendered conditionally.

Uhm interesting, I haven't thought of that prop as possibly being read like that, would it be better as analyticsDocsUrl?

sounds good to me.

25fb1b4 ✅

WiNloSt · 2024-03-20T09:58:48Z

frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepage.tsx

+      exampleDashboardId={1}
+      embeddingAutoEnabled={false}
+      licenseActiveAtSetup={true}
+      plan="oss-starter"


Should we add comments that these values should be from, if I'm not mistaken, settings?

That prop will be calculated in this component the same way as the urls, so it's kind of a "private prop".
It should make more sense in the following PRs, I should have left a comment that the values were temporarily hardcoded though

WiNloSt · 2024-03-20T10:30:50Z

frontend/src/metabase/home/components/EmbedHomepage/InteractiveTabContent.tsx

+
+      <Text fw="bold" mb="md">{t`The TL;DR:`}</Text>
+
+      <List


What are the reasons/benefits for using List here? I mean I understand Mantine has it, but this doesn't look anything different from using simple ol and li tags. If we need to use icons then maybe I could understand we might need List but that doesn't seem to be the case here.

Yeah itt felt overkill to me too but right now we have a reset that removes the list type, so we'd have to make a styled(ol)/css class with

list-style-type: decimal; list-style-position: inside;

I did it for the minimal embedding card but it felt like it should have been a re-usable component.

It should also help with making sure spacing, font sizes etc are consistent

WiNloSt · 2024-03-20T10:32:44Z

frontend/src/metabase/home/components/EmbedHomepage/InteractiveTabContent.tsx

+    exampleDashboardId,
+    licenseActiveAtSetup,
+    learnMoreInteractiveEmbedUrl,
+  } = props;


If we don't need to spread props like in frontend/src/metabase/home/components/EmbedHomepage/EmbedHomepageView.tsx what do you think about destructuring these props in the function arguments instead for consistency?

19d2486 ✅

WiNloSt · 2024-03-20T10:38:02Z

frontend/src/metabase/home/components/EmbedHomepage/InteractiveTabContent.tsx

+        // TODO: fix this in a better way
+        style={{ listStyleType: "decimal" }}
+      >
+        {licenseActiveAtSetup === false && (


I feel like booleans that could be undefined would be a bit awkward when the default value should be true. I'm not sure if that's the case, but the structure suggests so.

The reason sometimes we have someBoolean === false is to ensure undefined doesn't satisfy this clause. I feel like making the boolean that would have the default value as false would feel more natural. Since you can just do

Suggested change

{licenseActiveAtSetup === false && (

{!licenseActiveAtSetup && (

That being said, I guess from the name, this value should be false unless users activate their license, so I don't see the reason why we need to use xxx === false

I agree that's more readable, I think it may still make sense to set the default to undefined/null to make it explicit that the instance was setup after we introduced this, meaning that those flag have no value at all

to elaborate on what I meant by the default value as false. That covers the case for null and undefined too. I should have said falsy value by default.

It would feel more natural if you pass undefined, null, or false and gets the same result. Because It gets awkward sometimes if you pass undefined and false and get different results.

Ah I see, yeah that makes sense, I'll make sure not to render this when the any of the flags are undefined/null as it means they did the setup before this was introduced

…eTabContent.tsx Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>

* wip ui for the full embedding homepage * adds missing link + fix keys * remove done TODO * Update frontend/src/metabase/home/components/EmbedHomepage/InteractiveTabContent.tsx Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * destructure props in the function arguments * rename customerFacingAnalyticsDocsUrl to analyticsDocsUrl --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>

* create ui for the full embedding homepage, without any logic (#40251) * wip ui for the full embedding homepage * adds missing link + fix keys * remove done TODO * Update frontend/src/metabase/home/components/EmbedHomepage/InteractiveTabContent.tsx Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * destructure props in the function arguments * rename customerFacingAnalyticsDocsUrl to analyticsDocsUrl --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * add new settings for embedding homepage (#40455) * remove old minimal version * adds new embedding homepage settings and sets them at the end of the setup * comment e2e test * add tests for embedding homepage flags * more tests * remove unit tests related to old minimal version * fixes flags descriptions * :keyword for embedding-homepage Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> * replace reloadSettings and refreshSettingsList with initializeSettings * `value => value === true` -> `Boolean` * toMatchObject -> toEqual for more strict tests * remove useless undefined check for settings, the component is conditionally rendered based on a setting so it means they're loaded anyway * fix: use :hidden as we changed the type to :keyword * specifically check for the `embedding` feature in the tokenFeatures object instead of checking if any feature is active * remove double \n Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com> * connect settings to the embed homepage and show it (#40528) * fix nested a warnings in browser console * connect settings to the embed homepage and show it * fix stories * remove localStorage.clear() as we're not using it in the logic anymore * dismiss without feedback for the embedding homepage (#40587) * feat: add dismiss functionality to the embed homepage * update setup e2e to check for the dismissal of embedding homepage * use List from metabase/ui (#40638) * set embedding-secret-key when embedding is enabled (#40813) * set embedding-secret-key when embedding is enabled * use "named" setters and getters as suggested by Noah * Fix tests for ms1 after updating on master (#41069) --------- Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev> Co-authored-by: Noah Moss <32746338+noahmoss@users.noreply.github.com>

wip ui for the full embedding homepage

04e2020

metabase-bot bot assigned npretto Mar 18, 2024

metabase-bot bot added the .Team/Embedding label Mar 18, 2024

npretto changed the title ~~wip ui for the full embedding homepage~~ ui for the full embedding homepage Mar 19, 2024

npretto changed the title ~~ui for the full embedding homepage~~ create ui for the full embedding homepage, without any logic Mar 19, 2024

npretto mentioned this pull request Mar 19, 2024

[Epic] Initial homepage experience for embedding admins #40005

Closed

adds missing link + fix keys

ac6552c

npretto commented Mar 19, 2024

View reviewed changes

remove done TODO

4958615

npretto commented Mar 19, 2024

View reviewed changes

npretto requested a review from a team March 19, 2024 13:51

npretto marked this pull request as ready for review March 19, 2024 13:51

WiNloSt approved these changes Mar 20, 2024

View reviewed changes

npretto and others added 3 commits March 20, 2024 14:54

Update frontend/src/metabase/home/components/EmbedHomepage/Interactiv…

9791465

…eTabContent.tsx Co-authored-by: Mahatthana (Kelvin) Nomsawadi <me@bboykelvin.dev>

destructure props in the function arguments

19d2486

rename customerFacingAnalyticsDocsUrl to analyticsDocsUrl

25fb1b4

npretto merged commit c961b4d into embed-homepage Mar 20, 2024
77 of 78 checks passed

npretto deleted the embed-homepage-ui branch March 20, 2024 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

create ui for the full embedding homepage, without any logic #40251

create ui for the full embedding homepage, without any logic #40251

npretto commented Mar 18, 2024 •

edited

Loading

npretto Mar 19, 2024

WiNloSt Mar 20, 2024

npretto Mar 19, 2024 •

edited

Loading

npretto Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading

replay-io bot commented Mar 19, 2024 •

edited

Loading

WiNloSt left a comment

WiNloSt Mar 20, 2024

npretto Mar 20, 2024 •

edited

Loading

WiNloSt Mar 20, 2024

npretto Mar 20, 2024

WiNloSt Mar 20, 2024

npretto Mar 20, 2024

WiNloSt Mar 20, 2024

npretto Mar 20, 2024

WiNloSt Mar 20, 2024

npretto Mar 20, 2024

WiNloSt Mar 20, 2024

npretto Mar 20, 2024

WiNloSt Mar 20, 2024

npretto Mar 20, 2024

WiNloSt Mar 20, 2024

npretto Mar 20, 2024

		@@ -0,0 +1,47 @@
		import type { Meta, StoryObj } from "@storybook/react";

		@@ -0,0 +1,65 @@
		// eslint-disable-next-line no-restricted-imports
		import { List } from "@mantine/core";

	{licenseActiveAtSetup === false && (
	{!licenseActiveAtSetup && (

create ui for the full embedding homepage, without any logic #40251

create ui for the full embedding homepage, without any logic #40251

Conversation

npretto commented Mar 18, 2024 • edited Loading

Description

How to verify

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npretto Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

npretto Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Mar 19, 2024 • edited Loading

replay-io bot commented Mar 19, 2024 • edited Loading

WiNloSt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npretto Mar 20, 2024 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

npretto commented Mar 18, 2024 •

edited

Loading

npretto Mar 19, 2024 •

edited

Loading

npretto Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading

replay-io bot commented Mar 19, 2024 •

edited

Loading

npretto Mar 20, 2024 •

edited

Loading