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

fix: title and description #187

Closed

Conversation

PhilipWillms
Copy link
Contributor

@PhilipWillms PhilipWillms commented May 5, 2023

Closes #156

@vercel
Copy link

vercel bot commented May 5, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
blocknote ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 30, 2023 5:13pm
blocknote-website ❌ Failed (Inspect) May 30, 2023 5:13pm

@PhilipWillms
Copy link
Contributor Author

Discord example of how it looks now.
Screenshot 2023-05-05 at 19 55 54

@YousefED
Copy link
Collaborator

YousefED commented May 5, 2023

This is great @PhilipWillms! Thanks ❤️

Two questions I hope you can help with now you've already figured this out:

  • We can still override the title / description per page right? Because while this is a great improvement, all the pages will still have the same metadata
  • Same question for the og image. It's a bit overkill now to have it rendered like this, because there's no "dynamic" element. We might as well link to a static asset? Or, what would be even cooler is to add a dynamic title as well (like in this example, but that might be quite a bit more work

@PhilipWillms
Copy link
Contributor Author

PhilipWillms commented May 7, 2023

This is great @PhilipWillms! Thanks ❤️

Two questions I hope you can help with now you've already figured this out:

  • We can still override the title / description per page right? Because while this is a great improvement, all the pages will still have the same metadata
  • Same question for the og image. It's a bit overkill now to have it rendered like this, because there's no "dynamic" element. We might as well link to a static asset? Or, what would be even cooler is to add a dynamic title as well (like in this example, but that might be quite a bit more work

@YousefED I liked the idea of having images with dynamic titles so I implemented something like that. Now each page can have custom meta tags. I abstracted that in some way so that you do not need to set all meta tags on each page but rather just have to add some values in the frontmatter and the meta tags get created based on that. Not sure if we want it this way or rather be able to set all the meta tags for each site individually like it kinda got started in this commented code.

I was also not sure about how the preview image should look like. I did not like how it was looking when using the whole banner with the custom title below so I chose to just use the Icon like this.
Screenshot 2023-05-07 at 18 40 47

If no imageTitle got set the whole banner gets used as the og:image like this
Open for any adjustments. ✌️

Copy link
Collaborator

@YousefED YousefED left a comment

Choose a reason for hiding this comment

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

This is great @PhilipWillms, thanks!! I've checked the code and it looks great. Some final suggestions in the review 💪

{
property: "twitter:title",
content:
`${METADATA_DEFAULT.title} - ${pageData.frontmatter.title}` ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this syntax will work? if frontmatter.title is undefined, I think it will just render BlockNote - undefined right?

(there's a couple more occurences in this file)

</div>
);

return new ImageResponse(title ? iconWithTitle(title) : banner, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would probably prefer banner + title. Or alternatively, a quick fix could be to prefix the title with BlockNote - . Currently, there's no way from the image to understand to which site / project the image refers to which I think is not ideal

</svg>
<div
style={{
fontSize: 60,
Copy link
Collaborator

Choose a reason for hiding this comment

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

export default async function handler(request) {
try {
const { searchParams } = new URL(request.url);
const title = searchParams.get("title")?.slice(0, 100);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add ellipsis? … (single utf character) if it's too long?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Website: Fix titles and image previews
3 participants