Skip to content

refactor(react): use declarative HTML for known Text variants #1529

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.

Sign up for GitHub

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

Open
chenman9226 wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −37
Open

refactor(react): use declarative HTML for known Text variants #1529

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 25 additions & 33 deletions renderers/react/src/v0_9/catalog/basic/components/Text.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,54 +22,46 @@ import {useMarkdown} from '../hooks/useMarkdown';
// Import CSS Module
import styles from './Text.module.css';

/**
* Wraps the plain text with appropriate Markdown syntax based on the requested variant.
*
* @param text The plain text to be wrapped.
* @param variant The typography variant (e.g., 'h1', 'caption').
* @returns The text wrapped in Markdown syntax.
*/
const handleVariant = (text: string, variant?: string): string => {
switch (variant) {
case 'h1':
return `# ${text}`;
case 'h2':
return `## ${text}`;
case 'h3':
return `### ${text}`;
case 'h4':
return `#### ${text}`;
case 'h5':
return `##### ${text}`;
case 'caption':
return `*${text}*`;
default:
return text;
}
};
const NON_MARKDOWN_VARIANTS = new Set(['h1', 'h2', 'h3', 'h4', 'h5', 'caption']);
Copy link
Copy Markdown
Collaborator

@ditman ditman Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, add a JSDoc comment above this constant to explain what it's used for, something like: 'These variants are directly rendered with HTML markup, instead of being handled by the markdown processor' or similar!


export const Text = createComponentImplementation(TextApi, ({props}) => {
useBasicCatalogStyles();
const text = typeof props.text === 'string' ? props.text : String(props.text ?? '');
const markdownText = handleVariant(text, props.variant);
const renderedHtml = useMarkdown(markdownText);
const variant = props.variant;
const isKnownVariant = variant !== undefined && NON_MARKDOWN_VARIANTS.has(variant);
Copy link
Copy Markdown
Collaborator

@ditman ditman Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're not saving a lot by checking variant !== undefined before asking the set if it has it IMO. This could be simplified to just the has check.


const renderedHtml = useMarkdown(text);
Copy link
Copy Markdown
Collaborator

@ditman ditman Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not renderHtml(text) until it's needed, move this to under the if(isKnownVariant) (right when it's needed)

const style: React.CSSProperties = {
...getBaseLeafStyle(),
...getWeightStyle(props.weight),
};

const isCaption = props.variant === 'caption';
const classes = [styles.a2uiText, isCaption ? styles.a2uiCaption : props.variant || 'body'];
const isCaption = variant === 'caption';
const classes = [styles.a2uiText, isCaption ? styles.a2uiCaption : variant || 'body'];
Comment on lines +39 to +40
Copy link
Copy Markdown
Collaborator

@ditman ditman Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These seem to only be used inside if(isKnownVariant), can you move these two consts to inside the if block?


if (isKnownVariant) {
const HeadingTag = isCaption ? 'em' : (variant as 'h1' | 'h2' | 'h3' | 'h4' | 'h5');
if (isCaption) {
return (
<span className={classes.join(' ')} style={style}>
<HeadingTag>{text}</HeadingTag>
</span>
);
}
return (
<div className={classes.join(' ')} style={style}>
<HeadingTag>{text}</HeadingTag>
</div>
);
}

if (renderedHtml === null) {
classes.push('no-markdown-renderer');
}
const contentProps =
renderedHtml !== null
? {dangerouslySetInnerHTML: {__html: renderedHtml}}
: {children: markdownText};
: {children: text};

if (isCaption) {
return <span className={classes.join(' ')} style={style} {...contentProps} />;
}
Comment on lines -69 to -73
Copy link
Copy Markdown
Collaborator

@ditman ditman Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, I need to backport this to Lit as well!

return <div className={classes.join(' ')} style={style} {...contentProps} />;
});
Comment thread
chenman9226 marked this conversation as resolved.
2 changes: 1 addition & 1 deletion renderers/react/tests/v0_9/catalog-components.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ describe('Basic Catalog Components', () => {
const {view} = renderA2uiComponent(Text, 't1', {text: 'Title', variant: 'h1'});
const h1 = view.container.querySelector('div.h1');
expect(h1).not.toBeNull();
expect(h1?.textContent).toBe('# Title');
expect(h1?.textContent).toBe('Title');
Copy link
Copy Markdown
Collaborator

@ditman ditman Jun 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Chef's kiss!

});
});

Expand Down
6 changes: 3 additions & 3 deletions renderers/react/tests/v0_9/integration-scenarios.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ describe('Gallery Integration Tests', () => {
</React.StrictMode>,
);

expect(screen.getByText('### Markdown Rendering')).toBeInTheDocument();
expect(screen.getByText('Markdown Rendering')).toBeInTheDocument();
});

it('renders Task Card -> content visibility', async () => {
Expand All @@ -54,8 +54,8 @@ describe('Gallery Integration Tests', () => {
</React.StrictMode>,
);

expect(screen.getByText('### Review pull request')).toBeInTheDocument();
expect(screen.getByText('*Backend*')).toBeInTheDocument();
expect(screen.getByText('Review pull request')).toBeInTheDocument();
expect(screen.getByText('Backend')).toBeInTheDocument();
});

it('handles Login form -> input updates data model', async () => {
Expand Down