Skip to content
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

Markdown in descriptions #4405

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

dejanbelusic
Copy link
Contributor

@dejanbelusic dejanbelusic commented Dec 1, 2024

Update the feature to render markdown in the field description.

In the fields of type object and type boolean, markdown will be rendered in the description when enableMarkdownInDescription is set to true.

Issue reporting this bug: #3975
Previous PR to add the feature: #3665

The result:
Screenshot 2024-12-01 at 18 44 54

Checklist

  • I'm updating documentation
  • I'm adding or updating code
    • I've added and/or updated tests. I've run npx nx run-many --target=build --exclude=@rjsf/docs && npm run test:update to update snapshots, if needed.
    • I've updated docs if needed
    • I've updated the changelog with a description of the PR
  • I'm adding a new feature
    • I've updated the playground with an example use of the feature

When 'enableMarkdownInDescription' flag is set to true markdown will be rendered in the description
When 'enableMarkdownInDescription' flag is set to true markdown will be rendered in the description
Comment on lines 66 to 71
const richDescription = uiOptions.enableMarkdownInDescription ? (
<Markdown options={{ disableParsingRawHTML: true }}>{description || ''}</Markdown>
) : (
description || ''
);

Copy link
Member

@heath-freenome heath-freenome Dec 6, 2024

Choose a reason for hiding this comment

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

@dejanbelusic after looking around it turns out that every theme's CheckboxWidget has a similar description rendering logic in it. We suggest creating a new component in the @rjsf/utils package that wraps this logic as follows and then updating every FieldDescriptionTemplate to use it:

export interface RichDescriptionProps<T = any, S extends StrictRJSFSchema = RJSFSchema, F extends FormContextType = any> {
  /** The description of the field being rendered */
  description: ReactNode;
  /** The uiSchema that was passed to field */
  uiSchema?: UiSchema<T, S, F>;
  /** The `registry` object */
  registry: Registry<T, S, F>;
}
export default function RichDescription<T = any, S extends StrictRJSFSchema = RJSFSchema, F extends FormContextType = any>({ description, registry, uiSchema = {}}: RichDescriptionProps<T,S,F>) {
  const { globalUiOptions } = registry;
  const uiOptions = getUiOptions<T, S, F>(uiSchema, globalUiOptions);
  if (uiOptions.enableMarkdownInDescription) {
    return <Markdown options={{ disableParsingRawHTML: true }}>{description || ''}</Markdown>
 }
 return description;
}

With all the FieldDescriptionTemplates calling it similarly to the change that would be made in @rjsf/core's implementation:

export default function DescriptionField<
  T = any,
  S extends StrictRJSFSchema = RJSFSchema,
  F extends FormContextType = any
>(props: DescriptionFieldProps<T, S, F>) {
  const { id, description, uiSchema, registry } = props;
  if (!description) {
    return null;
  }
  return (
   <div id={id} className='field-description'>
      <RichDescription<T,S,F> description={description} uiSchema={uiSchema} registry={registry} />
    </div>
  );
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @heath-freenome, I see your point and I agree. I can create a new component RichDescription as suggested, place it in the @rjsf/utils, and then use it in every DescriptionField component.

When looking into DescriptionField components, I can see that one in @rjsf/antd packages is wrapped in a span, the one in @rjsf/bootstrap-4 is wrapped in two divs, the one in @rjsf/chakra-ui package is wrapped in Text component... What I'm saying is every theme has similar, but different implementation. Do I leave them as is?

Copy link
Member

@heath-freenome heath-freenome Jan 3, 2025

Choose a reason for hiding this comment

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

@dejanbelusic I would leave the different implementations of DescriptionField alone except render the RichDescription in place of description directly. And sorry for the late reply, I was on holiday for a few weeks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @heath-freenome, I commited the changes, please take a look if I understood you correctly.

Also, was looking into updating the tests, but I'm getting some errors, can you give me some pointers on how to troubleshoot them?

@dejanbelusic dejanbelusic marked this pull request as draft January 26, 2025 21:27
Copy link
Member

@heath-freenome heath-freenome left a comment

Choose a reason for hiding this comment

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

Looks good, some suggestions

Comment on lines +1 to +3
import getUiOptions from '../getUiOptions';
import { FormContextType, Registry, RJSFSchema, StrictRJSFSchema, UiSchema } from '../types';
import Markdown from 'markdown-to-jsx';
Copy link
Member

Choose a reason for hiding this comment

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

Please put global imports before local

Suggested change
import getUiOptions from '../getUiOptions';
import { FormContextType, Registry, RJSFSchema, StrictRJSFSchema, UiSchema } from '../types';
import Markdown from 'markdown-to-jsx';
import Markdown from 'markdown-to-jsx';
import getUiOptions from '../getUiOptions';
import { FormContextType, Registry, RJSFSchema, StrictRJSFSchema, UiSchema } from '../types';

Comment on lines +6 to +8
description: string;
uiSchema?: UiSchema<T, S, F>;
registry: Registry<T, S, F>;
Copy link
Member

Choose a reason for hiding this comment

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

Copy some appropriate documentation for each prop from one of the Props in the types.ts file please

Comment on lines +19 to +27
return (
<>
{uiOptions.enableMarkdownInDescription ? (
<Markdown options={{ disableParsingRawHTML: true }}>{description}</Markdown>
) : (
description
)}
</>
);
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified down to:

Suggested change
return (
<>
{uiOptions.enableMarkdownInDescription ? (
<Markdown options={{ disableParsingRawHTML: true }}>{description}</Markdown>
) : (
description
)}
</>
);
if (!uiOptions.enableMarkdownInDescription) {
return description;
}
return <Markdown options={{ disableParsingRawHTML: true }}>{description}</Markdown>;

Copy link
Member

Choose a reason for hiding this comment

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

You'll need to add unit tests for this in rjsf utils to pass the 100% coverage requirement

Comment on lines +30 to +33
## Dev / docs / playground

- Fixed typo in `package.json`

Copy link
Member

Choose a reason for hiding this comment

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

The package change is so small as to not require mentioning. What does want to be mentioned is that it is a new feature so requires 5.25.0 AND you want to mention the changes in each package.

@@ -541,7 +541,7 @@ export type DescriptionFieldProps<T = any, S extends StrictRJSFSchema = RJSFSche
/** The uiSchema object for this description field */
uiSchema?: UiSchema<T, S, F>;
/** The description of the field being rendered */
description: string | ReactElement;
description: string;
Copy link
Member

Choose a reason for hiding this comment

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

I believe this wants to be ReactNode as well. Which is what is causing the @rjsf/core tests to fail

@heath-freenome
Copy link
Member

@dejanbelusic Thanks for getting this closer to the finish line. I've left you some suggestions and comments

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants