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

Strip markdown from parameter placeholder #1002

Open
pmarschik opened this issue Oct 22, 2024 · 15 comments
Open

Strip markdown from parameter placeholder #1002

pmarschik opened this issue Oct 22, 2024 · 15 comments
Assignees
Labels
bug Something isn't working reviewing 👀 Undergoing manual audit to determine if issue should still be active

Comments

@pmarschik
Copy link

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

  • Version used: 4.1.0
@pmarschik pmarschik added the bug Something isn't working label Oct 22, 2024
@sserrata
Copy link
Member

sserrata commented Nov 5, 2024

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
Copy link
Author

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
Copy link
Contributor

slorber commented Nov 5, 2024

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
Copy link
Author

So whats happening in this plugin:

  • there is an OpenApi specification (OAS) document
  • some items of that document contain raw markdown text.
  • the plugin has a few React components that render these items
  • right now the raw markdown from the OAS document is rendered directly with react-markdown like <ReactMarkdown children={rawMarkdown} />

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
Copy link
Member

sserrata commented Nov 5, 2024

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
Copy link
Contributor

slorber commented Nov 6, 2024

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:

  • Which plugins do you need exactly?
  • What's the exhaustive list of behaviors that you want to port?
  • Why does this plugin do this at runtime instead of build time?

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
Copy link
Member

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

@sserrata sserrata added the reviewing 👀 Undergoing manual audit to determine if issue should still be active label Nov 22, 2024
@sserrata sserrata self-assigned this Nov 22, 2024
@pmarschik
Copy link
Author

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

  • when the description contains an admonition it kind of works but is not 100% compatible with docusaurus:
  • the description of a parameter, which is used in the ApiExplorer component in <ParamTextFormItem> can also contain markdown and is used like this in the theme:
     <FormTextInput
       isRequired={param.required}
       paramName={param.name}
       placeholder={param.description || param.name}
       onChange={(e: React.ChangeEvent<HTMLInputElement>) =>
         dispatch(
           setParam({
             ...param,
             value:
               param.in === "path" || param.in === "query"
                 ? e.target.value.replace(/\s/g, "%20")
                 : e.target.value,
           })
         )
       }
     />
    • The markdown in param.description still needs to be stripped as the placeholder attribute supports only plaintext

@sserrata
Copy link
Member

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
Copy link
Member

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

@sserrata
Copy link
Member

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
Copy link
Contributor

slorber commented Nov 22, 2024

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
Copy link
Member

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
Copy link
Member

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reviewing 👀 Undergoing manual audit to determine if issue should still be active
Projects
None yet
Development

No branches or pull requests

3 participants