prismicio / prismic-react

React components and hooks to fetch and present Prismic content
https://prismic.io/docs/technologies/homepage-reactjs
Apache License 2.0
154 stars 40 forks source link

Possible <RichText/> bug with spreading additional props #93

Closed TatisLois closed 2 years ago

TatisLois commented 2 years ago

I'm using the RichText from prismic-reactjs library in a typescript project and am trying to add a class name to the component like the first snippet below.

  return (
    <>
        <RichText render={title} className="some-class-name" />
    </>
  );

I am getting a Property 'className' does not exist on type 'IntrinsicAttributes & RichTextProps & { children?: ReactNode; }' error from TS and when reading the source file at node_modules/prismic-reactjs/index.d.ts I see that there is no explicit className prop or implicit spread of props.

    export const RichText: React.FC<RichTextProps> & {
        asText: (input: RichTextBlock[]) => string;
        render: (input: RichTextBlock[]) => React.ReactNode;
        displayName: "RichText";
    };

Is that a bug or expected behavior? I'm assuming a bug based on this issue that seems to have added support here: https://github.com/prismicio/prismic-reactjs/pull/41

a-trost commented 2 years ago

Yeah, this is definitely a bug. I hadn't seen that merge at #41 before, thanks for surfacing that. I'll see if we can get this fixed soon, it's definitely a point of frustration.

TatisLois commented 2 years ago

@a-trost no worries! Not sure how contribution-friendly the project is but I would also be open to taking a shot and opening a PR. It seems like the steps would be 1. Update the type definition 2. Ensure the props are spreading 3. Add a test case.

angeloashmore commented 2 years ago

Hey @TatisLois, a PR would be great if you see something that could be improved!

To give you an idea of what we're planning for the next version of this library, the <RichText> component will return an array of React elements. Providing something like className won't make sense in that case since there is no component wrapping the array of elements.

In the case where you want to add a class to a something wrapping the element, you would just add the wrapper in your code:

<div className="some-class-name">
  <RichText render={title} />
</div>

This gives you the flexibility to create the wrapper however you need, like using something different from a div, while acting exactly like any other component in your app.

The current version of the library works similarly, but it has the option to pass a Component prop. That prop adds a wrapper element which will spread any extra props, including className. By default, Component is a React fragment, which is why your className prop isn't seen.

<RichText render={title} Component="div" className="some-class-name" />
TatisLois commented 2 years ago

@angeloashmore that makes sense and that does work, thank you!

Seems like the type definition is still missing for className, it might be worth adding it as a conditional prop on the RichTextProps definition.

angeloashmore commented 2 years ago

@TatisLois You're right, the props are missing types for any of Component's props. We could do something like:

React.ComponentProps<typeof Component>

This will get all the props for whatever Component is, whether it's "div" or a custom React component. We just need to intersect that with the existing props. I'm happy to review a PR if you have time to submit one!

TatisLois commented 2 years ago

@angeloashmore I opened a PR and am opening to direction! I was able to get to work using a discriminating union not with React.ComponentProps<typeof Component>.

I'm opening to refactoring it and going with your idea but would need some guidance. I tried going down that route and could not figure it out.

angeloashmore commented 2 years ago

Hey @TatisLois, that's awesome! I just reviewed the PR and left a comment about how we could make the type more sound. I left some examples of how we can do what I explained above, but let me know if it's not working out and I can help. :)

angeloashmore commented 2 years ago

I'm going to close this as @prismicio/react v2's <PrismicRichText> component no longer renders a wrapping component. If you need a wrapping component with a specific class, just add an element around it.

Example:

<div className="some-class-name">
  <PrismicRichText field={document.data.example_rich_text} />
</div>

Thanks!