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

[docs-infra] Use edge function for card generation #41188

Merged
merged 34 commits into from Apr 10, 2024

Conversation

alexfauquette
Copy link
Member

@alexfauquette alexfauquette commented Feb 19, 2024

Closes #32036

The /edge-functions/og-image? 3 search params:

  • product: The text displayed next to the MUI logo
  • title, which can contain * to delimit the highlighted text sections
  • description a paragraph added under the main title
  • authors the GitHub username of the authors, divided by a coma.

Result of adding caching

Generating the card without caching (when you pass search params for the first time) take around 2s
Rendering card from the cache (when you pass search params already seen) take around 100ms

Note

The branding pages (for exemple mui.com) don't get a custom card. For those, I would be in favor of creating hand made ones, because they might be the most shared

Test on linkedin

https://deploy-preview-41188--material-ui.netlify.app/joy-ui/getting-started/
image

https://deploy-preview-41188--material-ui.netlify.app/joy-ui/main-features/dark-mode-optimization/
image

https://deploy-preview-41188--material-ui.netlify.app/joy-ui/integrations/material-ui/
image

https://deploy-preview-41188--material-ui.netlify.app/experiments/blog/blog-custom-card/
image

@alexfauquette alexfauquette added the scope: docs-infra Specific to the docs-infra product label Feb 19, 2024
@mui-bot
Copy link

mui-bot commented Feb 19, 2024

Netlify deploy preview

https://deploy-preview-41188--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against a570b9a

@alexfauquette alexfauquette changed the title [docs-infra] Test edge function for card generation [docs-infra] Use edge function for card generation Feb 27, 2024
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

A quick review

I recall facebook/react#13838, but It doesn't seem to be a problem anymore.

docs/src/modules/components/AppLayoutDocs.js Outdated Show resolved Hide resolved
docs/src/modules/components/AppLayoutDocs.js Outdated Show resolved Hide resolved
netlify/edge-functions/docs-image.tsx Outdated Show resolved Hide resolved
Comment on lines 1 to 7
/* eslint-disable no-console */
import React from 'https://esm.sh/react@18.2.0';
import { ImageResponse } from 'https://deno.land/x/og_edge/mod.ts';
/**
* The matching from github user to their full name
*/
const fullName: Record<string, string> = {
Copy link
Member

Choose a reason for hiding this comment

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

Do we need an edge function? It seems that a regular function could do the job.

Is there a way to not duplicate this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. It's on my "to explore" with the "should we use netlify CDN with it" 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

I searched a bit, but seems that functions are not able to support JSX which would make it much harder to define the image. Could be done with JSX object ({ type: 'div', props: { children: 'hello, world', style: { color: 'black' },},},) but not very good DX

The reason why I used edge function at first is because the package I'm using: https://github.com/ascorbic/og-edge is made to run on Netlify Edge Functions. Since the author works at Netlify I assumed he knows which kind of function is the best (not by best technical decision)

Copy link
Member Author

Choose a reason for hiding this comment

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

For the non duplication aspect of the fullName object, I'm not sure.

It's a bit the issue with those functions: they are all independent. But we could expose it in a URL and fetch it like this

import { fullName } from 'https://mui.com/public/contributors.json'

Copy link
Member

Choose a reason for hiding this comment

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

I searched a bit, but seems that functions are not able to support JSX which would make it much harder to define the image. Could be done with JSX object ({ type: 'div', props: { children: 'hello, world', style: { color: 'black' },},},) but not very good DX

Right, https://www.larocque.dev/projects/generative-open-graph-images/ says the same thing. We would need to add an intermediate step that converts JSX to JS. No preferences.

It's a bit the issue with those functions: they are all independent. But we could expose it in a URL and fetch it like this

This #41188 (comment) seems simple enough as a solution.

@alexfauquette
Copy link
Member Author

alexfauquette commented Mar 11, 2024

I've updated the description to match the current state of the PR

@alexfauquette alexfauquette marked this pull request as ready for review March 11, 2024 11:22
Comment on lines 147 to 159
<div
style={{
display: 'flex',
flexDirection: 'row',
flexWrap: 'wrap',
position: 'absolute',
bottom: 100,
left: 100,
right: 100,
}}
>
{authors &&
authors
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to render this whole div conditionally depending on whether there are authors or not? Just so the page's title is centered in case it's not a blog post (and thus without authors).

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It feels like we are close 👍

netlify/edge-functions/og-image.tsx Outdated Show resolved Hide resolved
@@ -8,6 +8,7 @@ export type MuiProductId =
| 'system'
| 'docs-infra'
| 'docs'
| 'toolpad'
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't exist anymore:

Suggested change
| 'toolpad'
| 'toolpad-core'
| 'toolpad-studio'

But I think should be in a different PR

authors: ['alexfauquette']
tags: ['Company']
card: false
cardTitle: blog with\n*custom* card
Copy link
Member

Choose a reason for hiding this comment

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

Smart

date: 2022-07-28T00:00:00.000Z
authors: ['alexfauquette']
tags: ['Company']
card: false
Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this? It would make sense for me to not throw is cardTitle is defined to take over.

Suggested change
card: false

I would also rename the other pages that use this so it's clear, something like this

Suggested change
card: false
manualCard: true

docs/pages/experiments/blog/blog-custom-card.md Outdated Show resolved Hide resolved
docs/src/modules/components/TopLayoutBlog.js Outdated Show resolved Hide resolved
netlify/edge-functions/og-image.tsx Outdated Show resolved Hide resolved
@oliviertassinari oliviertassinari added the enhancement This is not a bug, nor a new feature label Mar 21, 2024
@danilo-leal
Copy link
Contributor

Last idea from me — how about we render the <p class="description" of the docs on the image, too? I saw this in the Stripe docs OG image and felt like it is such a nice detail to quickly give more context to visitors.

Screenshot 2024-03-22 at 14 01 05

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Thanks for including the description! 🙏

netlify/edge-functions/og-image.tsx Outdated Show resolved Hide resolved
netlify/edge-functions/og-image.tsx Outdated Show resolved Hide resolved
Co-authored-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com>
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
@danilo-leal
Copy link
Contributor

They have not been written with this usecase in mind, and it's clearly visible. Description are long sentence.

They definitely haven't 😃 But it does seem to add value as a quick way to instruct readers what the page is about just from the OG image (particularly when tossed around Twitter). I think it's okay if we need to fine-tune some pages' descriptions given this newer "constraint".

But looking at the screenshot above, though, the whole thing seems to be using IBM Plex Sans now (the titles should still use General Sans). Also, I'd ask just to change the font-size/line-height a bit to 28px and 46px respectively; they ended up being too big, I think.

Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
@alexfauquette
Copy link
Member Author

the whole thing seems to be using IBM Plex Sans now

Fixed in commit b20e681 I forgot to put the correct name for this font 🙈 fixed now

@alexfauquette
Copy link
Member Author

I added

Copy link
Contributor

@danilo-leal danilo-leal left a comment

Choose a reason for hiding this comment

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

Sweet! In these last commits, I tweaked a bit the image's design a bit and added a StackBlitz link on the docs page with a reproduction of the image so we can more easily preview the design and test any visual adjustments there :) Looking awesome, thank you for working on this! 🤙

@alexfauquette alexfauquette merged commit 61e3fc4 into mui:master Apr 10, 2024
22 checks passed
alexfauquette added a commit to alexfauquette/material-ui that referenced this pull request Apr 10, 2024
Signed-off-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Co-authored-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
alexfauquette added a commit that referenced this pull request Apr 11, 2024
Signed-off-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Signed-off-by: Alexandre Fauquette <45398769+alexfauquette@users.noreply.github.com>
Co-authored-by: Danilo Leal <67129314+danilo-leal@users.noreply.github.com>
Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com>
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Great 👍


## Card design preview

Visit [this StackBlitz demo](https://stackblitz.com/edit/vitejs-vite-ukeejd?file=src%2FApp.tsx) to see how the card looks like without having to run a random page on an OG preview site.
Copy link
Member

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement This is not a bug, nor a new feature scope: docs-infra Specific to the docs-infra product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[docs-infra] OG images are not descriptive
4 participants