search

plone / volto

React-based frontend for the Plone Content Management System
https://demo.plone.org/
MIT License
426 stars 574 forks source link

[docs] improve style of Plone REST API JavaScript Client #5491

Open stevepiercy opened 4 months ago

stevepiercy commented 4 months ago

The recently added documentation for Plone REST API JavaScript client needs the following improvements.

Run vale docs/source/client to identify errors, warnings, and suggestions.

Most of the other messages may be ignored, but if you have any question, please ping @stevepiercy.

Current status:

✖ 55 errors, 28 warnings and 230 suggestions in 45 files.

See also https://github.com/plone/volto/pull/5490

Abro0058T commented 4 months ago

@stevepiercy the link is not working .

stevepiercy commented 4 months ago

@Abro0058T which link? The one in the issue description works for me.

Abro0058T commented 4 months ago

@stevepiercy maybe it was some browser issue

Abro0058T commented 4 months ago

@stevepiercy can you tell me the repo of this issue ?

stevepiercy commented 4 months ago

It's this repo because the issue is in this repo. I think you mean where in this repo, which would be inside /docs, specifically /docs/source/client.

Abro0058T commented 4 months ago

Aaaa...sorry i didn't look care fully .

stevepiercy commented 4 months ago

No worries. It's new to you, and that's why we're here, to point you in the right direction. That said, in most packages, documentation is usually located at the root of the repository in /docs.

Hrittik20 commented 4 months ago

Hello @stevepiercy, should I also fix all the "passive voice" errors?

stevepiercy commented 4 months ago

@Hrittik20 is it an error, warning, or suggestion?

In general, the passive voice issues should not be addressed at this time. Passive voice issues depend on the context. Sometimes the passive voice is OK.

Hrittik20 commented 4 months ago

Its mostly "suggestions". Also, I think I should skip some errors/warning such as "RelatedObject should use sentence-style capitalization" or "For a general audience, use address rather than URL". What do you think?

stevepiercy commented 4 months ago

@Hrittik20 I would focus only on errors at this time, but not all of them should be fixed. Vale is only as good as its configuration, and sometimes even returns false positives. We need to review the error message and decide whether to update Vale's configuration or follow its recommendation.

In the examples you provided, I would need to see the context, but in general:

If you can provide the full error message to include the file name and line number, or a link to the source files in the Volto repo where the text triggers these error messages, then I can provide exact guidance.

Hrittik20 commented 4 months ago

@stevepiercy I have fixed most of the errors (apart from a few) and also fixed many suggestions and warning. Should I make a PR now?

stevepiercy commented 4 months ago

@Hrittik20 yes, please, but first, please read and follow Contributing to Plone and First-time contributors.

We can take a look at the remaining errors either as a part of the pull request review or separately. Thank you!

Hrittik20 commented 4 months ago

@stevepiercy Okay. About the change log entry file, do I need to make it in the "Products.CMFPlone" repo after making this PR (since as per the documentation, I need to put it inside the news directory)?

stevepiercy commented 4 months ago

@Hrittik20 change log entries go in the same package as the docs you update, as stated in https://6.docs.plone.org/contributing/index.html#change-log-entry.

To create a change log entry or news item, create a file in the news directory, located in the root of the package.

For this package, it would be in:

...and duplicated to:

I'm not absolutely sure on the duplicate, because the monorepo thing is new. @sneridagh can you clarify whether we should duplicate change log files? I think we should so that they appear in the changelog when it is released, but I'll defer to your guidance. We might also need to update the https://6.docs.plone.org/contributing/index.html#change-log-entry docs explicitly for Volto.

Hrittik20 commented 4 months ago

@stevepiercy Okay, got it. Thanks for the guidance. Really appreciate it!

sneridagh commented 4 months ago

@stevepiercy It should not be duplicated if there's no change in packages/volto. There's no sense in that. If the docs apply generically to "docs for Volto" then it is.

In this case it's "docs for the client" which, in principle, has nothing to do with Volto the library itself. So, no changelog in Volto is required. Which sense would that have?

sneridagh commented 4 months ago

BTW, If there's any PR already involved with this one, please link it in the sidebar tools or mention it in the comments/description.

stevepiercy commented 4 months ago

@sneridagh the client docs are now included in the Volto docs, and that's why I thought it should be duplicated. But I understand how you see the client docs as being a self-contained sub-unit of Volto documentation.

stevepiercy commented 4 months ago

@sneridagh I forgot to ask, how do you communicate that a sub-project has a new release, assuming they still have a separate release now that they are in a monorepo? IOW, must one look at the sub-project's CHANGELOG.md, for example, https://github.com/plone/volto/blob/main/packages/client/CHANGELOG.md?

stevepiercy commented 4 months ago

I would like to add one more change request to this issue regarding each of the items listed under each Parameters heading. It will clean up a lot of the boilerplate markup and make it easier to style in the future. Apologies that I did not think of this sooner.

It requires a change to conf.py:

myst_enable_extensions = [
    "deflist",  # You will be able to utilise definition lists
    # https://myst-parser.readthedocs.io/en/latest/syntax/optional.html#definition-lists
    "fieldlist",  # For docstrings,
+   "linkify",  # Identify “bare” web URLs and add hyperlinks.
    "colon_fence",  # You can also use ::: delimiters to denote code fences,\
    #  instead of ```.
    "html_image",
]

And uses syntax as follows.

## Get add-on

```{js:function} getAddonQuery(addonId)

Use the `getAddonQuery` query function to get the query for fetching an add-on at a given path.

:arg string addonId: The add-on's ID.
:hook: `useGetAddon`


Example of before and after rendering.

<img width="884" alt="Screenshot 2023-12-28 at 11 06 13 AM" src="https://github.com/plone/volto/assets/102112/0d4d8197-d9c4-43e1-b969-3aa00bc6267f">

It also adds an entry to the automatically generated `genindex.html` file which hyperlinks to the defined function.

![Screenshot 2024-01-04 at 6 43 53 AM](https://github.com/plone/volto/assets/102112/0b24fdc0-9a90-4ca5-9d9b-b65d55830bb6)

See also [The JavaScript Domain](https://www.sphinx-doc.org/en/master/usage/domains/javascript.html) in Sphinx.
stevepiercy commented 3 months ago

@Hrittik20 what are your thoughts on my proposed change request?

Hrittik20 commented 3 months ago

@stevepiercy Okay, on it! Do we need to do it only for functions which has "parameters"? (e.g - "Get add-ons list" doesnt have parameters)

stevepiercy commented 3 months ago

It should be done for all functions. It will require a bit of shuffling around things to make it work.

For example, according to the The JavaScript Domain, we can use square brackets around an argument to indicate that it is optional, and avoid all that "Required: Yes/No" clutter. Arguments without brackets are implicitly required, and should be listed before optional.

I would suggest doing just one file to get a feel for it, and I can review just that one, before you go all out and make a bunch of changes that need to be changed yet again. Once we have an agreeable pattern, then you can go full speed ahead with the rest. Sound like a plan?