Support custom element and props in render

[elyobo]

The .render for rich text currently forces an additional div around the rendered elements, with no ability to customise the wrapper div nor any ability to pass props to that wrapper (e.g. a className).

As of React 16, components can render fragments so for React 16 the additional wrapper is not required. If you're keeping React 15 compatibility, and want to keep the same interface for consistency, it would be useful to allow options to be passed to configure the element type to wrap with and any additional props to pass at least, e.g.

  render(richText, linkResolver, htmlSerializer, { tagName = 'div', props = {}) = {}) {
    const serializedChildren = PrismicRichText.serialize(richText, serialize.bind(null, linkResolver), htmlSerializer);
    const _props = Object.assign(propsWithUniqueKey(), props);
    return React.createElement(tagName, _props, serializedChildren);
  }

[levimykel]

Hi @elyobo, thanks for bringing this up, you're absolutely right. We'll add this functionality.

[elyobo]

I realised after this that, as this isn't actually a component, it can happily return an array of elements even with React 15, which is possibly a cleaner approach - the user can then wrap (or not wrap) the result however they want.

You could return a single element if that's all that there was (e.g. a title field or a single item in a rich text field), or an array if there were multiple items; both can be rendered directly by React.

A related issue is the key for elements, that you're currently randomly generating - this is not a good idea, but you do have a problem with the key that you use for each top level element in the array that you return and I'm not sure I see a great way to do that. I really can't see any reason to be assigning key attributes to everything underneath that though.

[arnaudlewis]

Hi, react 15 doesn't allow you to render an array of components in few cases so we decided to wrap it just to simplify things for people and avoid covering corner cases on your own. You made a really good point about that and we'll try to implement the best possible solution to fix it for react 15 and 16. About the generated keys, I'm aware of what you're saying and you're right it's not ideal because we can't rely on the diff of vdom and everything will be fully rerender if something trigger a rerender in the component hierarchy but the only way would be to generate predictible keys so we wouldn't have that issue anymore. In addition to this, the component is rendered according to a tree of elements generated with the data from the API. It's completely dynamic and each node can have multiple children in the tree. Even if you serialize one node in this library, it's just a function that will be used multiple times to eventually serialize X nodes of the same type at the same level, which means that you'll have to define a unique key for each element. You're right it's not a perfect solution yet but I'd be happy to have your opinion on this and maybe find a way to solve all this.

On Wed, Nov 22, 2017, 21:45 elyobo ***@***.***> wrote: I realised after this that, as this isn't actually a component, it can happily return an array of elements even with React 15, which is possibly a cleaner approach - the user can then wrap (or not wrap) the result however they want. You could return a single element if that's all that there was (e.g. a title field or a single item in a rich text field), or an array if there were multiple items; both can be rendered directly by React. A related issue is the key for elements, that you're currently randomly generating - this is not a good idea <facebook/react#1342>, but you *do* have a problem with the key that you use for each top level element in the array that you return and I'm not sure I see a great way to do that. I really can't see any reason to be assigning key attributes to *everything* underneath that though. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFtiX8YwgvsEYromuRrpv_NXSgOm0W3_ks5s5IgFgaJpZM4Qm14a> .

-- Arnaud Lewis Prismic.io tel: 0787647471 email: arnaud@prismic.io

[elyobo]

Thanks @arnaudlewis,

The only array issue I was aware of with React 15 was that returning one directly from a render function was not possible (now supported in React 16), were there other issues as well? It does look like PrismicRichText.serialize will just return an array anyway... I understand wanting to keep the API simple to use without causing problems for users if there are other issues (and making the wrapper element configurable would keep it simple for most users while still allowing flexibility where it's needed). I would have thought that the no-array-return-from-render issue was well known enough that it alone would not be a justification for the wrapper though, but that decision is up to you guys :)

The key thing is a little tricky, but I think that for your particular case as one of the ones where just using the index as the key is probably fine; your content is static, it won't be reordered, and your components don't have any internal state. Small changes to prismic-richtext/serialize would allow this in a backwards compatible way by passing through the index through from fromRichText to serializeNode, then passing it in to step, e.g. something like this (disclaimer: I'm not familiar with TypeScript)

import Tree from './tree';
import { Node, SpanNode, NodeElement } from './nodes';
import { RichTextBlock } from './richtext';

type Serializer<T> = (type: string, element: NodeElement, text: string | null, children: T[]) => T;

function fromRichText<T>(richText: RichTextBlock[], serialize: Serializer<T>, htmlSerializer?: Serializer<T>): T[] {
  const tree = Tree.fromRichText(richText);
  return tree.children.map((node: Node, index: number) => {
    return serializeNode<T>(node, index, serialize, htmlSerializer);
  });
}

function serializeNode<T>(parentNode: Node, index: number, serializer: Serializer<T>, htmlSerializer?: Serializer<T>): T {

  function step(node: Node, idx: number): T {
    const text = node instanceof SpanNode ? node.text : null;
    const serializedChildren = node.children.reduce<T[]>((acc: T[], node: Node, i: number) => {
      return acc.concat([step(node, i)]);
    }, []);

    const maybeSerialized = htmlSerializer && htmlSerializer(node.type, node.element, text, serializedChildren);
    return maybeSerialized || serializer(node.type, node.element, text, serializedChildren, idx);
  }

  return step(parentNode, index);
}

export default fromRichText;

[arnaudlewis]

You're right it's a known issue and your issue will be a good occasion to talk about this and maybe remove the wrapper. About the key, it's a good lead to build a key based on the position in the tree so it would be predictable. The library is still young but we'll make sure to improve this part to fix this. If you have any more suggestions or if you simply want to contribute and make a pull request, it's more than welcome ;) Anyway we'll investigate these two subjects and get back to you so you can help us get the best out of this.

On Wed, Nov 22, 2017, 22:59 elyobo ***@***.***> wrote: Thanks @arnaudlewis <https://github.com/arnaudlewis>, The only array issue I was aware of with React 15 was that returning one directly from a render function was not possible (now supported in React 16), were there other issues as well? It does look like PrismicRichText.serialize will just return an array anyway... I understand wanting to keep the API simple to use without causing problems for users if there are other issues (and making the wrapper element configurable would keep it simple for most users while still allowing flexibility where it's needed). I would have thought that the no-array-return-from-render issue was well known enough that it alone would not be a justification for the wrapper though, but that decision is up to you guys :) The key thing is a little tricky, but I think that for your particular case as one of the ones where just using the index as the key is probably fine; your content is static, it won't be reordered, and your components don't have any internal state. Small changes to prismic-richtext/serialize <https://github.com/prismicio/prismic-richtext/blob/master/src/serialize.ts> would allow this in a backwards compatible way by passing through the index through from fromRichText to serializeNode, then passing it in to step, e.g. something like this (disclaimer: I'm not familiar with TypeScript) import Tree from './tree';import { Node, SpanNode, NodeElement } from './nodes';import { RichTextBlock } from './richtext'; type Serializer<T> = (type: string, element: NodeElement, text: string | null, children: T[]) => T; function fromRichText<T>(richText: RichTextBlock[], serialize: Serializer<T>, htmlSerializer?: Serializer<T>): T[] { const tree = Tree.fromRichText(richText); return tree.children.map((node: Node, index: number) => { return serializeNode<T>(node, index, serialize, htmlSerializer); }); } function serializeNode<T>(parentNode: Node, index: number, serializer: Serializer<T>, htmlSerializer?: Serializer<T>): T { function step(node: Node, idx: number): T { const text = node instanceof SpanNode ? node.text : null; const serializedChildren = node.children.reduce<T[]>((acc: T[], node: Node, i: number) => { return acc.concat([step(node, i)]); }, []); const maybeSerialized = htmlSerializer && htmlSerializer(node.type, node.element, text, serializedChildren); return maybeSerialized || serializer(node.type, node.element, text, serializedChildren, idx); } return step(parentNode, index); } export default fromRichText; — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFtiX6WO7N92l5a_kSW2ldVadTW9PofOks5s5JklgaJpZM4Qm14a> .

-- Arnaud Lewis Prismic.io tel: 0787647471 email: arnaud@prismic.io

[elyobo]

I'm happy to provide some PRs, given that you're open to changes here :)

The key only matters in relation to the other items in the list, so I don't think tracking the depth in tree as part of the key is required (otherwise it would be simple to pass the tree depth through each step as you recurse as well).

I'll open a PR against prismic-richtext for my suggestion there, if that's approved then we can use that to improve the key generation here.

Thanks for your responsiveness to this issue; we're currently evaluating Prismic for a project of ours and it's very encouraging!

[arnaudlewis]

Perfect I'll take a look. I'm a bit curious, what is your project? We are trying to be really responsive because we're convinced that it requires a strong community to build a great product :)

On Wed, Nov 22, 2017, 23:14 elyobo ***@***.***> wrote: I'm happy to provide some PRs, given that you're open to changes here :) The key only matters in relation to the other items in the list, so I don't think tracking the depth in tree as part of the key is required (otherwise it would be simple to pass the tree depth through each step as you recurse as well). I'll open a PR against prismic-richtext for my suggestion their, if that's approved then we can use that to improve the key generation here. Thanks for your responsiveness to this issue; we're currently evaluating Prismic for a project of ours and it's very encouraging! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFtiX2QaMSnkkiGROy7v7Twc9Svjxfbxks5s5JzZgaJpZM4Qm14a> .

-- Arnaud Lewis Prismic.io tel: 0787647471 email: arnaud@prismic.io

[tremby]

Would one way to allow custom props on the wrapper to provide an actual component? I was surprised not to find something like <PrismicRichText className="myClass" object={myRichTextObject} /> to be possible with this library when using JSX.

But since you guys are talking about possibly removing the wrapper entirely, that'd actually be preferable.

Eagerly awaiting a resolution to this.

[elyobo]

We were doing something similar to that internally and it's possibly a nice thing to provide as well. Something like this (untested)?

import React from 'react';
import { RichText } from 'prismic-reactjs';

const PrismicRichText = ({ tag = 'div', children, ...props }) => React.createElement(tag, props, RichText.render(children));

Use like

<PrismicRichText className="myClass" other="props">{prismicHtmlHere}</PrismicRichText>

[tremby]

By prismicHtmlHere you mean the rich text data structure, like response.results[0].data.my_richtext_field or whatever?

Sure, it'd be useful to include something like this. It should allow the linkResolver to be passed in too though. And passing in the data structure object as the component's children seems weird to me, since I'd expect to only put actual HTML or other components as something's children, but if that's a common pattern, so be it...

[elyobo]

Yeah, the rich text field.

Yes, link resolver needs to be passed through; would that be a global thing or would you pass it as a prop?

Passing as children is a taste thing I guess; I think it makes sense when a component is not actually going to render other children, it means you can set prop types or flow types appropriately to indicate what you actually expect there, but it's easy to add a richText prop or something instead and just ignore any children that are given.

[tremby]

Yes, link resolver needs to be passed through; would that be a global thing or would you pass it as a prop?

So far I'm just importing it from a module of my app wherever necessary and passing it into RichText.render. It's a little unwieldy. It'd be nice to register it (as a default but overridable linkResolver perhaps) with the Prismic tools so it's not necessary to pass each time.

[elyobo]

If it was accepted as a prop then it would be easy enough for you to create your own RichText component to wrap one provided here and pass in the right link resolver, then use your own component instead e.g.

import linkResolver from './my-link-resolver';
import PrismicRichText from 'prismic-reactjs/component';

const RichText = ({ children, ...props }) => <PrismicRichText {...props} linkResolver={linkResolver}>{children}</PrismicRichText>;

export default RichText;

Same eventual outcome (you only need to deal with your link resolver once).

[tremby]

Nice, I may try something like that. Thanks.

But I'd probably still rather have no wrapper!

[tremby]

Realized for now I can just do {RichText.render(page.heading, linkResolver).props.children} to unwrap the contents.

[elyobo]

@tremby I suggest opening another issue if you'd like a render component or registration of a default link resolver.

[tremby]

I think registration of a default link resolver is possibly best done with a per-project wrapper as you suggested.

As for a component, I'm not sure if it's super necessary or helpful. I was mostly just surprised there wasn't already one.

[tremby]

Where did we land on ditching the wrapper element? The PR you opened doesn't address that.

[elyobo]

No, I didn't want to mix up the concerns in that PR so have left it for the moment.

[mathieudelvaux]

Hi guys, any news on this?
The issue was closed but I can't find any PR addressing this.
I'm using @tremby's solution (props.children) for the moment.

Although, I have a similar issue. My custom type is setup this way:

"myText": {
    "type": "StructuredText",
    "config": {
      "single": "strong",
      "label": "My Text"
    }
  },

Paragraphs are supposed to not be allowed, but my structured text is still rendered inside a <p> tag, inside a <div>, obviously.