🐛 [BUG] - Too strict validations prevent acceptance of valid outputs about brand

[rezrah]

Potential solution:

Use the planned Prose component to render native html markup as a child of River, which will also have the restrictions removed.

This would assume that markdown has been pre-processed.

[sergioalvz]

@rezrah, do you have any estimates for when the Prose component will be ready? This is blocking a few work streams we want to complete by the next two weeks.

[rezrah]

@sergioalvz - sorry for late response. We're drafting the prerequisite issues for Prose this week, eng to begin shortly after. ETA for the component in the published library is just an estimate at this point, but we can expedite release to within the next two weeks.

[rezrah]

👋 @sergioalvz - quick update for you on this topic following a discussion at Primer Brand maintainer sync on 10th July.

We discussed the trade-offs around increasing responsibility and scope of River.Content, carefully weighing up pro's and cons..

• Decision was that enabling Prose at this time inside River.Content presents too much of a design pattern risk. For example, Prose supports more elements than River.Content is intended to handle, E.g. multiple paragraphs, multiple headings etc. The unpredictability of the input could lead to unintentionally facilitating anti-patterns.

• The preferred approach for using Markdown input going forward is as described in an earlier post:

  • Parsing the markdown AOT and mapping the relevant parts of the AST to River components like Heading, Text and Link
  • The conversion of markdown isn't a responsibility of the library at this time, as we avoid taking heavy third-party dependencies to ensure maximum interop with a wider range of projects. As a result, we would expect the conversion to happen AOT. Happy to jam with you on a few different ways you could do this though! LMK.

• Re. input from Contentful.. could you create a simple text-only mapping between dedicated heading, description and link to ensure that we only pass in the necessary data to the River.Content layer?

[josepmartins]

Decision was that enabling Prose at this time inside River.Content presents too much of a design pattern risk. For example, Prose supports more elements than River.Content is intended to handle, E.g. multiple paragraphs, multiple headings etc. The unpredictability of the input could lead to unintentionally facilitating anti-patterns.

Heads up: we are currently working on design specs for the River component were we define that the content should be short and concise (3 or 4 sentences tops), and avoid nested headings or multiple paragraphs as that is not the intended usage for that component.

[raytalks]

@sergioalvz Quick update on the Prose component: we have started to work on the API exploration and do some research but unfortunately won't be able to ship within 2 weeks due to scheduled OOO of Primer Brand team.

cc @JoshBowdenConcepts @enstyled @rezrah

[rezrah]

@waynewi @sergioalvz - quick update that Prose has shipped in 0.20.1.

[rezrah]

👋 @sergioalvz - Prose can't be used in River.Content just yet, as that component guards the children types to Heading, Text and Link as you mentioned. Anything besides those children are ignored.

To clarify why that decision was made.. it's to ensure visual consistency of the River pattern across all marketing pages, mitigating situations where we see inconsistently sized (or levelled) headings, or even buttons that end up being used instead of links.

The use-case for Prose is limited to situations where you can't use or map against React components. I.e. you need to dangerouslySetInnerHTML for editorial text. Even if you could use Prose in the River.Content, you would lose the ability to guardrail presentation, as the CMS could any heading level (should be h3), and the paragraph text won't inherit default styling (slightly larger than the default).

I think we could solve for the use-case in the description in a few different ways:

1. Allow Prose as a valid child of River.Content in Primer Brand, and apply the appropriate styles internally. We can action this, and is also why the issue is still open for us.

2. Use a markdown parser function instead of a React component

   I recommend pre-processing the markdown outside of the return statement. Using a library like marked, you can avoid using a React component (which gets ignored) and pass the converted markdown directly to Prose, which requires html input.

   import {parse} from 'marked';
   
   parse('hola, this is just a [test](#) description for a River.'); // "<p>hola, this is just a <a href='#'>test</a> description for a River.</p>"

   Then pass ☝️ straight into Prose. This helps us to maintain required guardrails around valid types, while adding new, vetted components to the safelist like Prose. It's also more performant, because you can memoise that value now.

3. Extract content in a markup-agnostic way and use Primer Brand components natively

   The preferred approach - if possible - would be to extract data from the CMS without additional markup, so that you could pass the raw text directly into typographic components. Is it possible to retrieve this from Contentful using independent fields? The closer we can get to using the recommended API the better 🙏. We'll still look at enabling Prose in River in the interim. cc. @JoshBowdenConcepts @raytalks

[sergioalvz]

Thanks for the context, @rezrah!

In the long term, we are heading towards option 3) since Contentful provides a way to directly map RichText content to React elements.

Supporting markdown-generated content inside River elements would be handy in the short/mid-term since some pages don't benefit from Contentful support yet and are developed without direct access to Primer Brand primitives. Developers use a custom River component that forwards the content to Primer Brand's River component. We considered improving the interface for this custom River component to support rich text formatting by leveraging Markdown. I guess only option 1) would help on that front.

To provide some more context, we created these custom abstractions on top of Primer Brand components to make sure they are easy to connect with Contentful later this year and that the pages created following this approach are easy to migrate as well (because they are constrained to strict limitations in customization and styling).

[waynewi]

@raytalks and @rezrah Could you please provide an update?

[sergioalvz]

@rezrah, great news! Is it possible to use the Prose component inside other components (for example, inside the River.Content component)?

[sergioalvz]

Thank you, folks, and thank you for your time yesterday. I think we're cool closing this issue and parsing the Markdown AOT. With the latest shift in priorities, we'll connect things to Contentful sooner than we expect, so this becomes less of a problem (I think we won't be leveraging Markdown in the short term, either).