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

docs: refactor API documentation #5219

Merged
merged 17 commits into from
Aug 4, 2021

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Jul 26, 2021

Motivation

While working on the documentation, I found that the API documentation is quite hard to follow with a giant code block. This PR attempts to use tables consistently for all API.

As I continued working on the refactoring, I realized that this will also resolve #3643.

Have you read the Contributing Guidelines on pull requests?

Yes

Test plan

Changes include:

Related PRs

Initially documentation for #5203, but when I merged this branch I found some docs already written 😂 Anyways I also enhanced related docs

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Jul 26, 2021
@netlify
Copy link

netlify bot commented Jul 26, 2021

✔️ [V2]

🔨 Explore the source changes: 50a24d4

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/610aa1fe11e88600089654d8

😎 Browse the preview: https://deploy-preview-5219--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Jul 26, 2021

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 32
🟢 Accessibility 98
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 95

Lighthouse ran on https://deploy-preview-5219--docusaurus-2.netlify.app/

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Copy link
Contributor

@lex111 lex111 left a comment

Choose a reason for hiding this comment

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

Nice improvement! However I would fix the following ones:

  • Element table not allowed as child of element small in this context. And in general it makes little sense to reduce font size in table at all.
  • It is also better to move Default column right after Type. Many docs follow exactly this ordering of columns in similar tables (name | type | default | description).

@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Jul 26, 2021

  • Element table not allowed as child of element small in this context. And in general it makes little sense to reduce font size in table at all.

Agree that it looks awkward, but using regular font would over-compress the column width of the "description" field and make the layout even more awkward... The other doc sites I've seen use a smaller font size (Ant Design, Vuetify, etc.). Should I use styling instead?

  • It is also better to move Default column right after Type. Many docs follow exactly this ordering of columns in similar tables (name | type | default | description).

Absolutely, makes sense

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@lex111
Copy link
Contributor

lex111 commented Jul 26, 2021

Yes, but small font is uncomfortable to read, so it's better use regular font size. Let's wait for opinion from @slorber then.

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
| `sidebarItemsGenerator` | `SidebarGenerator` | _Omitted_ | Function used to replace the sidebar items of type `'autogenerated'` by real sidebar items (docs, categories, links...). See also [Customize the sidebar items generator](/docs/sidebar#customize-the-sidebar-items-generator) |
| `numberPrefixParser` | <code>boolean &#124; PrefixParser</code> | _Omitted_ | Custom parsing logic to extract number prefixes from file names. Use `false` to disable this behavior and leave the docs untouched, and `true` to use the default parser. See also [Using number prefixes](/docs/sidebar#using-number-prefixes) |
| `docLayoutComponent` | `string` | `'@theme/DocPage'` | Root Layout component of each doc page. |
| `docItemComponent` | `string` | `'@theme/DocItem'` | Main doc container, with TOC, pagination, etc. |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Need some help here, I don't really understand the difference between DocPage and DocItem even after reading the code

Copy link
Collaborator

Choose a reason for hiding this comment

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

DocPage is more like the comp responsible for handling the routing and layout inside a doc version.

Keep descriptions as is, I'll try to figure out how to organize this better later and try to rename those comps

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@slorber
Copy link
Collaborator

slorber commented Jul 27, 2021

Thanks, that's a good improvement and will also fix some searchability issues due to having options only documented in code blocks!

Yes, but small font is uncomfortable to read, so it's better use regular font size. Let's wait for opinion from @slorber then.

Small looks better to me, but I understand why it can be an a11y concern. At the same time it remains big enough and users can still use their browser zoom? Not using small will penalize all users with a less readable layout so I'd suggest to keep small

Will try to review this in depth later

Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
website/docs/blog.md Outdated Show resolved Hide resolved
slorber and others added 2 commits August 4, 2021 16:15
# Conflicts:
#	website/docs/api/themes/theme-configuration.md
@slorber
Copy link
Collaborator

slorber commented Aug 4, 2021

looks good, thanks 👍 and it solves some searcheability issues we have with algolia + codeblocks.

Going to merge. Open to refine the table/small thing is someone has something better to suggest.

It is also possible that this small HTML item can mess up with Crowdin docs translations: we'll see after merging.

@slorber slorber merged commit c603056 into facebook:master Aug 4, 2021
@Josh-Cena Josh-Cena deleted the sidebar-section-doc branch August 4, 2021 14:35
@slorber slorber added the pr: documentation This PR works on the website or other text documents in the repo. label Aug 4, 2021
@Josh-Cena
Copy link
Collaborator Author

Josh-Cena commented Aug 4, 2021

It seems to me that adding a module css may not be a bad choice, but we can wait and see since the current solution has less code

@slorber
Copy link
Collaborator

slorber commented Aug 4, 2021

It seems to me that adding a module css may not be a bad choice, but we can wait and see since the current solution has less code

I don't understand what you mean 😅

@Josh-Cena
Copy link
Collaborator Author

I mean, adding an api.module.css with:

.apiTable {
  font-size: 10px;
}

And then apply this class to every table that we wrap with <small />

@slorber
Copy link
Collaborator

slorber commented Aug 5, 2021

Having an <ApiTable> mdx component could work instead of wrapping with <small>

If possible I'd prefer to use simple markup without React props because it tends to mess-up a bit with Crowdin without using ```mdx-code-block escaping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: documentation This PR works on the website or other text documents in the repo.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Indicate blog customization in the documentation
4 participants