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

Properly handle custom block types with children #54

Closed
wants to merge 2 commits into from

Conversation

fritz-c
Copy link
Contributor

@fritz-c fritz-c commented Mar 31, 2023

#29 most clearly defines the issue that this PR fixes (which I found only after doing an identical investigation with identical findings myself). In short, the logic to pass portable text blocks to the appropriate renderer uses a check, isPortableTextBlock, that is too lenient in its definition of a block for the context it's used in, and any custom block (e.g., _type: "testimonial") that contains a children value that looks like inline blocks or is an empty array will get treated as if they were of _type === "block".

Definitely:
Fixes #29
Pretty confidently:
Fixes #37
Probably:
Fixes #38

@rexxars
Copy link
Member

rexxars commented Apr 3, 2023

Thanks for the PR!

Some thoughts I'd like to discuss - /cc @portabletext/javascript:

Currently, all blocks produced by the Sanity portable text editor use _type: 'block'. However, this is not necessarily true for the future, in a world where we want to easily differentiate between customized block types in a typesafe way.

For instance, a portable text block that does not allow any annotations on its text nodes, only allows text nodes as children, and only allows the style of the node to be one of the heading levels (h1 through h6) could be thought of (and typed as) headingBlock, whereas a portable text block that allows text annotations such as links and article references and allows custom inline nodes (say, characterReference) might be typed as richArticleBlock.

In these cases, we'd actually want to have the _type attribute be headingBlock and richArticleBlock. While this is currently not the case, it is a goal we want to move towards, since it makes knowing what can appear in a block much more transparent and straightforward, without doing type narrowing which could result in wrong types being inferred (for instance if a richArticleBlock was added to a document, and its style was set as a heading, but no annotations were added - it would technically qualify as both richArticleBlock and headingBlock, but the intention is for it to be rendered as a richArticleBlock).

With this in mind, I was hoping to stay away from explicit _type === 'block' checks, as it is not very future-compatible. I have a branch that builds on this PR that instead just prioritizes any defined component from types: 3a8a41b

While this approach solves not being able to override rendering for custom types, it still maintains the behavior where block-like types are automatically rendered as portable text blocks. The question I have is whether or not this is developer friendly behavior, or if it would be better to require the user to specify all their custom block types, if any. Either through us exposing the default block rendered as (say) DefaultBlock and having people import it and define it in their types, or by a new prop defining which types to treat as blocks:

import {PortableText, DefaultBlock} from '@portabletext/react'

<PortableText
  // {...}
  components: {
    types: {
      myCustomBlock: DefaultBlock
    }
  }
/>

or

import {PortableText} from '@portabletext/react'

const blockTypes = ['block', 'myCustomBlock']

<PortableText
  // {...}
  // prop name up for discussion, eg `blockTypes`, `renderAsBlocks` or similar
  blockTypes={blockTypes}
/>

@fritz-c
Copy link
Contributor Author

fritz-c commented Apr 4, 2023

@rexxars Thank you for the background. That makes sense then, that the existing check is how it is.

I do like the idea of prioritizing the types components, as well as exposing the default renderer. The latter would let users add little extras to the rendering without having to recreate the entire block rendering logic themselves or somehow redirecting the data back into a nested PortableText component. For example, you could have a _type of "quote" that renders the portable text adjacent to a big quotation mark icon or something like that, but that still allows for use of marks in the text content rendered via DefaultBlock.

You can close this PR, though I would appreciate it if you pointed me towards an issue you'll track this with (#29, maybe) so I can follow it.

@snorrees
Copy link
Contributor

@rexxars I'd be happy with the approach in used in 3a8a41b

Since having a default block renderer is already established, I think having it "just work" is the way forward.
Exporting it is also a good idea, since that would allow devs to decorate the default component if needed.

Ie, I prefer this myself:

import {PortableText, DefaultBlock} from '@portabletext/react'

<PortableText
  // {...}
  components: {
    types: {
      myCustomBlock: DefaultBlock
    }
  }
/>

@rexxars
Copy link
Member

rexxars commented Apr 18, 2023

Sounds good to me @snorrees - feel free to build on this branch: https://github.com/portabletext/react-portabletext/tree/feat/allow-override-block-like

@snorrees
Copy link
Contributor

snorrees commented Apr 26, 2023

@snorrees snorrees closed this Apr 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants