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 Generator #8903

Draft
wants to merge 49 commits into
base: dev
Choose a base branch
from

Conversation

jperson2000
Copy link
Sponsor Contributor

@jperson2000 jperson2000 commented May 7, 2024

Description

This update refactors the API Documentation generator to add several features:

  • Strongly typed documentation objects for ApiPage.razor to use
  • Improved trimmability by removing all Reflection code from Blazor pages
  • Support for "documentation coverage" metrics shown during builds

The new documentation generator has these features:

  • Support for <summary> and <remarks> elements
  • Support for the <c> element (converted to <code>)
  • Support for the <para> element (converted to <p>)
  • Support for HTML in XML documentation comments
  • Support for links to members in the same class or any base class
  • Support for <see cref links

This update also adds Documentation Coverage metrics during builds:

XML Documentation Coverage for MudBlazor:

Types:      69 of 169 (41%) other types
Properties: 1458 of 2451 (59%) properties
Methods:    170 of 2093 (8%) methods
Fields:     12 of 667 (2%) fields/enums
Events:     0 of 6 (0%) events

API Builder: WARNING: 280 types have XML documentation which couldn't be matched to a type.
API Builder: WARNING: 569 properties have XML documentation which couldn't be matched to a type's property.
API Builder: WARNING: 653 methods have XML documentation which couldn't be matched to a type's property.
API Builder: WARNING: 4 events have XML documentation which couldn't be matched to a type's property.
API Builder: WARNING: 176 fields have XML documentation which couldn't be matched to a type's property.

This update also makes a few minor code cleanup changes:

  • The Paths class is now static

image
image

How Has This Been Tested?

The TestsForApiPages test generator was updated to use the new Api Blazor page when generating tests. A big change is that we now test all public types, not just types inheriting from MudComponentBase. This resulted in an additional ~400 tests being generated during builds.

Also, per #3595, an additional bank of tests are now generated which test the legacy API URL's. This will ensure that users who have bookmarked the old API page will still work properly.

The total API docs tests increased from ~150 to ~600.

Type of Changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation (fix or improvement to the website or code docs)

Checklist

  • The PR is submitted to the correct branch (dev).
  • My code follows the code style of this project.
  • I've added relevant tests.

@github-actions github-actions bot added the docs Related to docs label May 7, 2024
Copy link

codecov bot commented May 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.47%. Comparing base (28bc599) to head (ff309e0).
Report is 245 commits behind head on dev.

Current head ff309e0 differs from pull request most recent head 68d3b78

Please upload reports for the commit 68d3b78 to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev    #8903      +/-   ##
==========================================
+ Coverage   89.82%   90.47%   +0.64%     
==========================================
  Files         412      398      -14     
  Lines       11878    12375     +497     
  Branches     2364     2403      +39     
==========================================
+ Hits        10670    11196     +526     
+ Misses        681      628      -53     
- Partials      527      551      +24     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@danielchalmers
Copy link
Contributor

danielchalmers commented May 7, 2024

Just checking: does it support <br/>? I've been using that recently instead of <para> because it's more compact. para might want a little more space in the docs compared to br (but much less than before), what do you think?

@ScarletKuro
Copy link
Member

Would you able to do this testing #3595
To see that nothing unexpected got broken?

@ScarletKuro
Copy link
Member

ScarletKuro commented May 7, 2024

Would you able to do this testing #3595 To see that nothing unexpected got broken?

Maybe I'd be good idea to leave the code of this in the tool folder so anyone could run it at anytime.

@iwis
Copy link
Contributor

iwis commented May 7, 2024

Why did you change LayoutProperties.DefaultBorderRadius to DefaultBorderRadius in the description of MudChipset.Label?

Some incorrect descriptions:
MudChipSet<T>.Comparer
MudHighlighter.Markup
MudMessageBox.YesButton

I only checked a few pages manually. When I was refactoring the code for API documentation, often some change was manifested only in a bug in a few properties on a few pages. This is why I wrote #3595 to be sure I don't break documentation. As you see, unfortunately, manual testing isn't enough :(

@jperson2000
Copy link
Sponsor Contributor Author

Just checking: does it support <br/>? I've been using that recently instead of <para> because it's more compact. para might want a little more space in the docs compared to br (but much less than before), what do you think?

I'm pretty sure <br/> and other tags would be supported since the new code uses MarkupString. I'll be sure to test that.

@jperson2000
Copy link
Sponsor Contributor Author

Why did you change LayoutProperties.DefaultBorderRadius to DefaultBorderRadius in the description of MudChipset.Label?

Some incorrect descriptions: MudChipSet<T>.Comparer MudHighlighter.Markup MudMessageBox.YesButton

I only checked a few pages manually. When I was refactoring the code for API documentation, often some change was manifested only in a bug in a few properties on a few pages. This is why I wrote #3595 to be sure I don't break documentation. As you see, unfortunately, manual testing isn't enough :(

Thanks for checking some pages! I agree, it's really tough to get every detail just right, like dealing with reflection names. Hopefully I can get some automated tests built from your great test code.

@jperson2000 jperson2000 changed the title Docs: Refactor API Documentation Pages to Support XML Documentation Tags Docs: Refactor API Documentation Generator May 8, 2024
@jperson2000
Copy link
Sponsor Contributor Author

Alrighty, after almost a month, I think this Docs refactor is nearly ready to review! There are some things I still have to improve like documenting method parameters and enums/fields, but I think the site is in good shape to look at. Here are the major differences to look for:

  • The API docs exist for all public types, not just types inheriting from MudComponentBase. This helps to cover non-component documentation such as TableState and others.
  • The API test generator now generates tests for all public types. The tests assert that documentation was actually found for the page. For types which are Mud components, the tests also assert that an example link was found.

Let me know if you'd like any UI changes, and I'll bring this out of draft once I have the last pieces done in about 2-3 days. Thanks @ScarletKuro @henon @danielchalmers @mckaragoz @Anu6is for your valuable feedback as always!

Steps to Test

  1. From this branch, run MudBlazor.Docs.Compiler to generate new docs and tests on your machine.
  2. Change the startup project to Server or WASMHost and run.
  3. Navigate to any API component page.

@danielchalmers
Copy link
Contributor

Thanks for taking into account so much of my feedback, I appreciate that a lot :)
Everything looking great, big improvement over the v6 docs.

image

image

On mobile:

  • Horizontal padding limits amount of info that can be seen
  • Header is right-to-left (normally it's on the left side)
  • The select ("Name") doesn't have a hint about its function
  • May need very slightly more vertical padding between rows so they don't blend into each other

Accessibility (cc @igotinfected what do you think? screenshots above):

  • Should you be able to tab between rows?
  • Contrast between code blocks and background is low in dark mode
    • Swapping the row hover and default background color might work
    • Also might help to replace the code blocks with simple color text (aka remove the background)
  • Aria labels ("Property name", "Property type", "Property description")?

Other:

  • I think I was wrong when I said to skip documenting RightToLeft because I didn't think about it being in the docs

@jperson2000
Copy link
Sponsor Contributor Author

jperson2000 commented May 27, 2024

Suggestions for improvements

Great catches as always, @danielchalmers ! I removed the pa-8 on tables for better display on mobile devices, and added aria-label for table headers. The right-aligned table group seems like the MudTable default for xs sizes.

image
image

@jperson2000
Copy link
Sponsor Contributor Author

jperson2000 commented May 27, 2024

@henon I added API docs tests for all of the legacy API URL's based on #3595. Thanks again for the reminder, and I still have your suggestion on my list to add public types to the main MudBlazor search as part of this PR.

An example of failed API URL tests: https://github.com/MudBlazor/MudBlazor/actions/runs/9249343454/job/25441037707?pr=8903

@henon
Copy link
Collaborator

henon commented May 27, 2024

Great, I'll check out your code and run it for a thorough review soon.

@igotinfected
Copy link
Contributor

igotinfected commented May 27, 2024

Awesome work on this PR!

@danielchalmers

Should you be able to tab between rows?

Looks like the docs are generated with a MudTable so improving accessibility on that component should cover that use-case. Unsure if you need to be able to tab between rows/cells, but at a minimum you should be able to navigate through the sortable headers to change the sorting.

Aria labels ("Property name", "Property type", "Property description")?

Aria labels are only required if there are otherwise no visible textual labels. Don't think they're mandatory here!

Some other FYI's (haven't read through the entire conversation, apologies if they are known issues):

  • I had to build the MudBlazor.Docs.Comiler project manually to generate the ApiDocumentation.Generated.cs file to be able to run the docs project
  • Some example pages seem to throw:
    Table
    image
    DataGrid
    image
  • The URL from the API page back to the Examples page is broken (for generic components I assume) as it adds DataGrid`1 to the URL:
    image
    image

@iwis
Copy link
Contributor

iwis commented May 27, 2024

Below is not a comprehensive review, but only a report of a few issues that I noticed without a longer analysis, which is still needed.

Things that are less user-friendly in the new doc:

  • In the previous UI, if I wanted to know what is the difference between https://mudblazor.com/api/textfield and https://mudblazor.com/api/numericfield I switched from "Categories" to "Inheritance" and I saw that:
    MudTextField has additional fields: Clearable, AutoGrow, InputType, Mask, MaxLines;
    MudNumericField has additional fields: Clearable, HideSpinButtons, InvertMouseWheel, Min, Max, Step.
    That way I was able to learn about these two component easily.
    In the new UI you have removed the "Categories/Inheritance/None" switch, so how can I learn this in the new UI?
  • On https://localhost:5001/api/MudAutocomplete%601 it would be better if the properties categories are in the old order:
    Data
    Validation
    Behavior
    Appearance
    List behavior
    List appearance
    Common
    The order has a crucial meaning - more meaningful than the alphabetical one.
  • On https://localhost:5001/api/MudColorPicker there is no "Default" column in the "Properties" table. So for example one cannot simply find out which icon is default for some icon property. On https://mudblazor.com/api/colorpicker you can see the look of the icon, so it is easier to connect the property to the real piece of the Color picker component that you have seen in the examples.
  • On https://localhost:5001/api/MudAutocomplete%601 there is no information about the elements of enumeration type "Margin" in the "Margin" property (None, Dense, Property).
  • On the https://dev.mudblazor.com/api/autocomplete there is a clock "Rendered in 138 ms". I don't see such a clock on https://localhost:5001/api/MudAutocomplete%601, so we cannot compare if the pages are slower in the new UI. This is bad. In the beginning, the pages were loading in 2 seconds. We need the counter to monitor the loading times - we don't want pages to load again in 2 seconds and the 2 sec time is very easy to obtain incrementally without the counter,

Bugs:

On https://localhost:5001/api/MudAutocomplete%601 why are there event callbacks "OnKeyDown" and "OnKeyUp" in the "Properties" section and not in the "EventCallbacks" section?? Why did you remove the EventCallbacks section??

Generally, you removed my two improvements:
#2823
#3172
but why?

@henon
Copy link
Collaborator

henon commented May 28, 2024

  • On https://localhost:5001/api/MudAutocomplete%601 it would be better if the properties categories are in the old order:
    Data
    Validation
    Behavior
    Appearance
    List behavior
    List appearance
    Common
    The order has a crucial meaning - more meaningful than the alphabetical one.

I think this needs more argumentation why this order is supposed to be so meaningful. I supect it is based on personal preference. Others may say that Appearance, List appearance, Behavior, List behavior, Data, Validation, ... etc is a better order. In the end, alphabetical is a good order if people can not agree on a "correct" order.

@iwis
Copy link
Contributor

iwis commented May 28, 2024

@henon: Maybe it is a personal preference. I hoped that for many the current order could be beneficial. I could argue why the current order is beneficial, but on the other hand, if it is beneficial then the benefits should be visible at least for some of us. So instead of arguing I just asked here: #9077 what you prefer. If nobody prefers the current order then it is obvious that it was only my preference.
I raised this because I am still not sure if the alphabetical order is proposed consciously or only by accident or simply because it is easier to program.

@iwis
Copy link
Contributor

iwis commented May 29, 2024

Sorry, I meant the #9077 discussion above.

@jperson2000
Copy link
Sponsor Contributor Author

Below is not a comprehensive review, but only a report of a few issues that I noticed without a longer analysis, which is still needed.

Thanks for the valuable feedback, @iwis ! This is a draft PR with a ground-up rewrite of the docs compiler so that we can access documentation from anywhere in the MudBlazor docs web site via models, and without Reflection. Thus, a lot of work has to be done to restore features like the ones you mentioned. Please don't think I'm "taking features away" from you; on the contrary, I'm taking all feedback like yours into account to make sure this isn't a step backwards when it's finally ready for a formal review in a few weeks. I'll tag you for additional feedback once I get more features implemented.

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

Successfully merging this pull request may close these issues.

None yet

6 participants