Skip to content

fix: support direct image hyperlinks #640

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/notion-types/src/block.ts
Original file line number Diff line number Diff line change
Expand Up @@ -277,6 +277,9 @@ export interface ToggleBlock extends BaseBlock {

export interface ImageBlock extends BaseContentBlock {
type: 'image'
format: {
image_hyperlink: string
} & BaseContentBlock['format']
Comment on lines +280 to +282
Copy link

Choose a reason for hiding this comment

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

The image_hyperlink property should be marked as optional since it won't be present on all image blocks. Consider updating the type definition to:

format?: {
  image_hyperlink?: string
} & BaseContentBlock['format']

This ensures type safety when working with image blocks that don't contain hyperlinks.

Suggested change
format: {
image_hyperlink: string
} & BaseContentBlock['format']
format?: {
image_hyperlink?: string
} & BaseContentBlock['format']

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

}

export interface EmbedBlock extends BaseContentBlock {
Expand Down
84 changes: 56 additions & 28 deletions packages/react-notion-x/src/components/asset-wrapper.tsx
Original file line number Diff line number Diff line change
@@ -1,12 +1,25 @@
import type React from 'react'
import { type BaseContentBlock, type Block } from 'notion-types'
import { type BaseContentBlock, type Block, type ImageBlock } from 'notion-types'
import { parsePageId } from 'notion-utils'

import { useNotionContext } from '..'
import { cs } from '../utils'
import { Asset } from './asset'
import { Text } from './text'

interface PageInfo {
type: 'page'
id: string
url: string
}

interface ExternalInfo {
type: 'external'
url: string
}

type URLInfo = PageInfo | ExternalInfo | null

const urlStyle = { width: '100%' }

export function AssetWrapper({
Expand All @@ -19,18 +32,11 @@ export function AssetWrapper({
const value = block as BaseContentBlock
const { components, mapPageUrl, rootDomain, zoom } = useNotionContext()

let isURL = false
if (block.type === 'image') {
const caption: string | undefined = value?.properties?.caption?.[0]?.[0]
if (caption) {
const id = parsePageId(caption, { uuid: true })
const caption = value.properties?.caption?.[0]?.[0]
const imageHyperlink = (value as ImageBlock).format?.image_hyperlink
const linkToUse = imageHyperlink || caption || ''

const isPage = caption.charAt(0) === '/' && id
if (isPage || isValidURL(caption)) {
isURL = true
}
}
}
const urlInfo = getURLInfo(value, linkToUse)

const figure = (
<figure
Expand All @@ -41,8 +47,8 @@ export function AssetWrapper({
blockId
)}
>
<Asset block={value} zoomable={zoom && !isURL}>
{value?.properties?.caption && !isURL && (
<Asset block={value} zoomable={zoom && !urlInfo}>
{value?.properties?.caption && (
<figcaption className='notion-asset-caption'>
<Text value={value.properties.caption} block={block} />
</figcaption>
Comment on lines +51 to 54
Copy link

Choose a reason for hiding this comment

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

The caption is now always displayed, even when it's being used as a link URL. This appears to contradict the PR description which mentions "exposing the link as a caption feels intrusive and affects the user experience."

Consider restoring the condition to hide captions when they're used as links:

{value?.properties?.caption && (!urlInfo || imageHyperlink) && (
  <figcaption className='notion-asset-caption'>
    <Text value={value.properties.caption} block={block} />
  </figcaption>
)}

This would hide the caption when it's being used as a link URL (unless imageHyperlink is also present), which aligns with the stated goal of the PR.

Suggested change
{value?.properties?.caption && (
<figcaption className='notion-asset-caption'>
<Text value={value.properties.caption} block={block} />
</figcaption>
{value?.properties?.caption && (!urlInfo || imageHyperlink) && (
<figcaption className='notion-asset-caption'>
<Text value={value.properties.caption} block={block} />
</figcaption>
)}

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

Expand All @@ -52,23 +58,21 @@ export function AssetWrapper({
)

// allows for an image to be a link
if (isURL) {
const caption: string | undefined = value?.properties?.caption?.[0]?.[0]
const id = parsePageId(caption, { uuid: true })
const isPage = caption?.charAt(0) === '/' && id
const captionHostname = extractHostname(caption)
if (urlInfo?.url) {
const urlHostName = extractHostname(urlInfo.url)
const isExternalLink =
urlHostName && urlHostName !== rootDomain && !caption?.startsWith('/')

const href =
urlInfo.type === 'page' && urlInfo.id
? mapPageUrl(urlInfo.id)
: urlInfo.url

return (
<components.PageLink
style={urlStyle}
href={isPage ? mapPageUrl(id) : caption}
target={
captionHostname &&
captionHostname !== rootDomain &&
!caption?.startsWith('/')
? 'blank_'
: null
}
href={href}
target={isExternalLink ? 'blank_' : null}
>
{figure}
</components.PageLink>
Expand All @@ -77,6 +81,31 @@ export function AssetWrapper({

return figure
}
function getURLInfo(block: BaseContentBlock, link?: string): URLInfo {
if (!link || block.type !== 'image') {
return null
}

const id = parsePageId(link, { uuid: true })
const isPage = link.charAt(0) === '/' && id

if (isPage) {
return {
type: 'page' as const,
id,
url: link
}
}

if (isValidURL(link)) {
return {
type: 'external' as const,
url: link
}
}

return null
}

function isValidURL(str: string) {
// TODO: replace this with a more well-tested package
Expand All @@ -91,7 +120,6 @@ function isValidURL(str: string) {
)
return !!pattern.test(str)
}

function extractHostname(url?: string) {
try {
const hostname = new URL(url!).hostname
Expand Down