-
Notifications
You must be signed in to change notification settings - Fork 1
Implement Native SDK RichText Component #125
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements a native SDK RichText component for the Optimizely CMS SDK, providing a safe and customizable way to render rich text content from Slate.js JSON format.
- Adds a framework-agnostic
BaseRichTextRenderer
abstract class with shared rendering logic - Implements a React-specific RichText component that extends the base renderer
- Updates documentation and sample sites to demonstrate usage with custom element overrides
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/optimizely-cms-sdk/src/components/richText/ |
Core framework-agnostic rich text renderer with base class, types, and utilities |
packages/optimizely-cms-sdk/src/react/richText/ |
React-specific implementation with component, renderer, and utilities |
packages/optimizely-cms-sdk/src/model/properties.ts |
Removes outdated comment about RichText type |
packages/optimizely-cms-sdk/src/infer.ts |
Updates type inference for RichText properties with proper JSON structure |
packages/optimizely-cms-sdk/package.json |
Adds new export path for React richText module |
samples/nextjs-template/src/components/AboutUs.tsx |
Demonstrates RichText component usage with custom element renderer |
docs/6-rendering-react.md |
Updates documentation to showcase RichText component instead of dangerouslySetInnerHTML |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation looks good! I have some thoughts:
- Can we add unit tests to check that the component works as expected?
- Maybe in a future, we want to make a "JSX" abstraction between the React and the framework-independent layers. It would depend on how popular is JSX outside of React and if how easy/hard is to implement without using React-specific features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job modelling the JSON structure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks a good 2-layer abstraction (generic & React-specific)!
> [!TIP] | ||
> Using the `<RichText/>` component is the recommended way to render rich text content. It's safer than `dangerouslySetInnerHTML` as it doesn't rely on HTML parsing, and allows you to customize how elements are rendered with your own React components. | ||
### Customizing Rich Text Elements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to consider re-organizing the documentation, because right now we only have a "getting started tutorial".
My feeling is that this part is a bit too advanced for that.
Don't delete the documentation because it's very valuable :), we just need to think where to move it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Great work!
url?: string; // common on 'link', 'image', 'video' | ||
class?: string; // allow headless CMS to pass CSS classes | ||
[key: string]: unknown; // custom attributes | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any element types that we should create "super" types for? I get that this type covers all element types now, but e.g. our image
and link
elements might have attributes we want to have typed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an interesting question. If we opt to do so, we must define type
as a set of constants (i.e. type: "paragraph" | "link" | "image"...
). That will mean we need to know all possible values for type
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we will have to maintain individual types like this right?.
export interface LinkElement extends BaseElement {
type: 'link';
url: string; // Required for links - will be mapped to href
target?: '_blank' | '_self' | '_parent' | '_top';
rel?: string;
title?: string;
}
export interface ImageElement extends BaseElement {
type: 'image';
url: string; // Required for images - will be mapped to src
alt?: string;
title?: string;
width?: number | string;
height?: number | string;
}
export type Element =
| GenericElement
| LinkElement
| ImageElement
| TableCellElement;
I can add them since we kinda know what are the type
we get from the RichText in CMS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now we define the "super" types like LinkElement
, ImageElement
and its attributes. If by chance we get a type like 'unknown-element'
from graph (CMS's RichText) we render it using a regular div (we can override with our own element) which is a fallback component.
… element behavior
… improved readability
…ext component in samples and documentation
8b11060
to
c4d2b5a
Compare
BaseRichTextRenderer
abstract class with common rendering functions and add Types (Slate.js format).<RichText/>
usage and update sample site.