PaloAltoNetworks / docusaurus-openapi-docs

🦝 OpenAPI plugin for generating API reference docs in Docusaurus v3.
https://docusaurus-openapi.tryingpan.dev
MIT License
723 stars 239 forks source link

Strip markdown from parameter placeholder #1002

Open pmarschik opened 1 month ago

pmarschik commented 1 month ago

Describe the bug

Have a parameter defined for some path. That parameter can have a description which according to the OAS spec supports CommonMark syntax.

This description is used by packages/docusaurus-theme-openapi-docs/src/theme/ApiExplorer/ParamOptions/ParamFormItems/ParamTextFormItem.tsx (and ParamArrayFormItem.tsx) as placeholder on the <FormTextInput>.

This would show raw markdown if description contains markdown (e.g. a raw markdown link such as leading [Some link](/pointing/somewhere) trailing.

Expected behavior

Strip the markdown to plaintext so the placeholder text is readable. The correctly rendered markdown is anyways in the main section.

So leading [Some link](/pointing/somewhere) trailing should become leading Some link trailing.

Current behavior

Raw markdown is rendered as text.

Possible solution

Maybe use something like https://github.com/remarkjs/strip-markdown to strip non markdown elements.

Your Environment

sserrata commented 3 weeks ago

Hi @pmarschik, this is likely related to #1012. Basically, we need to decide whether we want to re-introduce remark rendering of descriptions/summaries or if Docusaurus could possibly provide a hook or method for rendering Docusaurus-style MDX client-side.

pmarschik commented 3 weeks ago

Yes, except for the detail that #1012 is about 4.2 and I tested this with 4.1.

I'd love for docusaurus to expose their MDX parsing facility client-side so the same markdown could be used. I know that the OAS spec just specifies CommonMark but having the same abilities as Docusaurus would be awesome.

I've just had a cursory look at the docusaurus code and all of their parsing seems to be in the @docusaurus/mdx-loader package. However, not enough is exposed to clients to be able to use that. Maybe @slorber has plans to expose more of the functionality there? (or a JSX component where one could pass raw MDX that gets rendered by Docusaurus).

slorber commented 3 weeks ago

I'm not sure to understand the problem, but overall we don't really plan to ship an MDX parser/renderer to the client side 😅 This would increase the weight of the client-side application. We don't prevent you from doing it yourself though (MDX can run in a browser), but beware of the tradeoffs you make.

pmarschik commented 3 weeks ago

So whats happening in this plugin:

But it would be awesome to get access to the configured MDX processor to render that raw markdown text. That way all configured remark/rehype plugins would work, as well as being able to use all configured mdx components.

sserrata commented 3 weeks ago

Thanks @pmarschik, I think that's the basic approach/idea - that the plugin could somehow use the custom remark plugins located here: https://github.com/facebook/docusaurus/tree/main/packages/docusaurus-mdx-loader/src/remark

slorber commented 3 weeks ago

Unfortunately, it's unlikely to happen.

Those remark plugins have various dependencies, some of them don't even work in a browser. Some plugins even use fs.readFile. User-provided plugins might also depend on Node.js env.

At best, we can expose a subset of the plugins, but it's impossible to expose all of them and expect things to work in a browser.

The questions are:

Shipping react-markdown to the browser is expensive. Maybe it could pre-parse the md on the Node.js side and pass the AST to the browser. That makes the system more lightweight and still gives the flexibility for a theme to provide custom components to render Markdown elements.

sserrata commented 6 days ago

Hi @pmarschik, now that #1016 is merged I was hoping you could help test the latest canary. Thanks!

pmarschik commented 5 days ago

@sserrata I've tested with 0.0.0-953, and it only partially fixes the problem:

sserrata commented 5 days ago

Hi @pmarschik, I wasn't able to reproduce the admonition issue concerning the leading/trailing new lines. Can you share the full reproduction details in case I am missing something?

As for the placeholder, we could just potentially opt to default to the param.name value only, since the param.description could potentially contain MDX/JSX, etc. Thoughts?

sserrata commented 5 days ago

Alternatively, we could also first try to default to param.example before defaulting to name which may make more sense.

sserrata commented 5 days ago

Also, FWIW, quick test of strip-markdown shows it doesn't strip admonitions or tables by default:

Screenshot 2024-11-22 at 10 04 09 AM
slorber commented 5 days ago

Directives are not std CommonMark, you probably need to use this syntax extension with strip-markdown: https://github.com/remarkjs/remark-directive

Similarly, tables are a remark-gfm feature afaik: https://github.com/remarkjs/remark-gfm

sserrata commented 1 day ago

Yup, so the question is whether or not it's worth stripping all three, CommonMark, admonitions and GFM, in order to continue using descriptions as placeholders.

For sure, I think the description is the most useful option to display but users can also find the full description in the ApiItem panel column, so we're essentially duplicating it in ApiExplorer column. I suppose mobile users could benefit more from displaying it in the ApiExplorer column since they'd have to scroll to see it otherwise in the smaller viewport.

Also, since descriptions could be multiline and/or longer than can be displayed, it may also make sense to strip the first line and set a max character limit.

Lastly, I was thinking we may also want to pass/support a defaultValue prop and set it to the example value if one exists, but this would, of course, "hide" the description.

sserrata commented 1 day ago

Actually...let me qualify what I mean by "most useful option to display/use"...

If the goal is to explain/teach then the description is likely the most helpful/useful content to use as a placeholder.

If the goal is to provide a "ready to send" example payload for demonstrating the API than example is the most helpful/useful content to set as the defaultValue.

Maybe there's a way to support/display/use both, but I still feel like we would be duplicating at least some of the description which could add unnecessary visual noise. Interested in additional thoughts/feedback here.

pmarschik commented 1 day ago

Hi @pmarschik, I wasn't able to reproduce the admonition issue concerning the leading/trailing new lines. Can you share the full reproduction details in case I am missing something?

As for the placeholder, we could just potentially opt to default to the param.name value only, since the param.description could potentially contain MDX/JSX, etc. Thoughts?

I might have missed a rebuild step in my testing, hence the issue with the leading/trailing lines. Anyways, I can work around that by updating the yaml files.

Concerning what to display as a placeholder, I'll quote MDN:

The placeholder attribute defines the text displayed in a form control when the control has no value. The placeholder text should provide a brief hint to the user as to the expected type of data that should be entered into the control.

Effective placeholder text includes a word or short phrase that hints at the expected data type, not an explanation or prompt. — https://developer.mozilla.org/en-US/docs/Web/HTML/Attributes/placeholder

So according to that definition example would make the most sense. If discoverability for mobile users is a concern, a separate paragraph or popover with properly styled markdown would probably work better thant including a potentially long description in the placeholder. Or, this part could be skipped completely.