Feat/110 generic embed #115
Conversation
src/Router.tsx
Outdated
| { | ||
| path: '/us/ai', | ||
| element: <AIDemoPage />, | ||
| }, |
There was a problem hiding this comment.
issue, blocking: We're not looking to embed this as a page at this point.
Please remove this from the routes. We're looking to create reusable design components at the moment, not actually code pages yet.
src/components/Embed/Embed.tsx
Outdated
| import React from "react"; | ||
| import { Card, Stack, Title, Text } from "@mantine/core"; | ||
|
|
||
| type Kind = "youtube" | "vimeo" | "iframe" | "image"; |
There was a problem hiding this comment.
nit/suggestion: "kind" is a pretty non-standard term for something like this. "type" is more standard, but obviously quite vague and a reserved keyword. I'd recommend something like "embedType"
src/components/Embed/Embed.tsx
Outdated
| type Props = { | ||
| header: string; | ||
| kind: Kind; | ||
| src: string; // iframe src or image src | ||
| description?: string; // optional text under header | ||
| aspectRatio?: `${number} / ${number}` | number; // default 16/9 | ||
| alt?: string; // required if kind="image" | ||
| }; |
There was a problem hiding this comment.
issue: While you can describe this as a type, it's more standard to use an interface here
One drawback of types is that you lose the model name when using IntelliSense. Since you're not actually reusing this structure anywhere except the model of the inbound props, I'd recommend interface here
src/components/Embed/Embed.tsx
Outdated
|
|
||
| type Props = { | ||
| header: string; | ||
| kind: Kind; |
There was a problem hiding this comment.
nit: Here, again, kind is fairly non-canonical. Would recommend something more standard.
src/components/Embed/Embed.tsx
Outdated
| const fixSrc = (kind: Kind, src: string) => { | ||
| if (kind === "youtube") { | ||
| const id = | ||
| src.match(/(?:v=|youtu\.be\/|\/embed\/)([A-Za-z0-9_-]{6,})/)?.[1] ?? null; |
There was a problem hiding this comment.
suggestion: Define this regex elsewhere as a const and pass into .match() as an arg
It's tough to read these inline
src/components/Embed/Embed.tsx
Outdated
|
|
||
| const fixSrc = (kind: Kind, src: string) => { | ||
| if (kind === "youtube") { | ||
| const id = |
There was a problem hiding this comment.
suggestion/nit: ID is often used to refer to records in a database. Instead, call this link.
src/components/Embed/Embed.tsx
Outdated
| aspectRatio = "16 / 9", | ||
| alt, | ||
| }: Props) { | ||
| const normalized = fixSrc(kind, src); |
There was a problem hiding this comment.
suggestion/nit: Normalization often refers to structuring data in such a way that it's easy to find different record types by their IDs. Instead, I'd refer to this as "modified," I guess? Or something else.
src/components/Embed/Embed.tsx
Outdated
| <div style={{ maxWidth: 960, margin: "0 auto" }}> | ||
| <h2 style={{ margin: "0 0 6px 0" }}>{header}</h2> | ||
| {description ? ( | ||
| <p style={{ margin: "0 0 10px 0", color: "#6b7280" }}>{description}</p> | ||
| ) : null} | ||
|
|
||
| <div | ||
| style={{ |
There was a problem hiding this comment.
issue, blocking: Please use Mantine v8 components when building. You can find their docs at https://mantine.dev/.
Please also use styling from the design framework. Our preference on styling is overrides to defaults -> defined "variants" in the styles folder -> local overrides
src/components/Embed/Embed.tsx
Outdated
| ); | ||
| } | ||
|
|
||
| export default Embed; |
There was a problem hiding this comment.
nit: Super minor, but my preference is to just use ES6+ "export default function Embed". Not necessary, though.
src/pages/AIDemo.page.tsx
Outdated
| import { Embed } from "../components/Embed/Embed"; | ||
|
|
||
| export default function AIDemoPage() { | ||
| return ( | ||
| <div style={{ padding: 24 }}> | ||
| <Embed | ||
| header="Watch Our AI Demo" | ||
| kind="youtube" | ||
| src="https://www.youtube.com/watch?v=fnuDyLKpt90" // replace with real ID | ||
| /> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
issue, blocking: Please remove, as we are just looking to create generic components at the moment.
Migrate the "Watch Our AI Demo" component on https://policyengine.org/us/ai page to App v2 so that it can handle generic Embed components (videos, images, or other external embed content).