MudBlazor / MudBlazor

Blazor Component Library based on Material design with an emphasis on ease of use. Mainly written in C# with Javascript kept to a bare minimum it empowers .NET developers to easily debug it if needed.
http://mudblazor.com
MIT License
7.22k stars 1.18k forks source link

MudCheckBox/MudChip/MudChipSet/MudCollapse: Added XML Documentation for Public Members #8829

Closed jperson2000 closed 2 weeks ago

jperson2000 commented 2 weeks ago

Description

This update adds XML documentation for public members, for the following components and classes:

This update also fixes a default text issue in the ColorPickerPage.razor example page.

This update also fixes a minor documentation issue in MudAppBar, changing "a value indicating whether" to just "whether".

How Has This Been Tested?

This was tested by observing the XML documentation for examples:

MudCheckBox

image image image image

MudChip

image image image image

MudChipSet

image image image image

MudCollapse

image image

Type of Changes

Checklist

jperson2000 commented 2 weeks ago

Here's another batch of documented components @danielchalmers @henon @mikes-gh when you get time. I'm going to hold off documenting MudColorPicker until #8826 is completed, to avoid merge conflicts. Thanks!

codecov[bot] commented 2 weeks ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.11%. Comparing base (28bc599) to head (0f50c06). Report is 125 commits behind head on dev.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## dev #8829 +/- ## ========================================== + Coverage 89.82% 90.11% +0.28% ========================================== Files 412 421 +9 Lines 11878 12215 +337 Branches 2364 2410 +46 ========================================== + Hits 10670 11008 +338 + Misses 681 665 -16 - Partials 527 542 +15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

danielchalmers commented 2 weeks ago

I know this is kind of late but I wonder if "Gets or sets" is necessary. If it's a Parameter it's almost always going to support those two things so it's kind of just duplicated text.

"Gets or sets the text to display next to the checkbox." could be "The text to display next to the checkbox."

"Gets or sets whether the appbar will be placed at the bottom of the screen instead of the top." -> "Whether the appbar will be placed at the bottom of the screen instead of the top."

More concise and takes up less space in the docs (same reasoning as before). This is just my opinion. What do you think? Still looks good in general!

jperson2000 commented 2 weeks ago

I know this is kind of late but I wonder if "Gets or sets" is necessary. If it's a Parameter it's almost always going to support those two things so it's kind of just duplicated text.

"Gets or sets the text to display next to the checkbox." could be "The text to display next to the checkbox."

"Gets or sets whether the appbar will be placed at the bottom of the screen instead of the top." -> "Whether the appbar will be placed at the bottom of the screen instead of the top."

More concise and takes up less space in the docs (same reasoning as before). This is just my opinion. What do you think? Still looks good in general!

That's a good point since MudBlazor components have almost entirely Get/Set properties. "Gets or sets" used to be enforced via StyleCop SA1623, but that was 7 years ago and there don't seem to be any modern Code Analysis rules for XML documentation in regards to phrasing these days. Even Blazor's own XML documentation shows no standard, with CascadingValue using summaries with just "The", whereas ChangeEventArgs uses "Gets or sets".

If others @henon @ScarletKuro @mikes-gh can give your opinion on this that would be great. Should we make public property summaries just start with "The" instead of "Gets or sets" to slim down the docs? If so, I will use that convention going forward and will fix up previously documented classes so everything is consistent.

henon commented 2 weeks ago

"Whether the appbar will be placed at the bottom of the screen instead of the top."

Problem is, this is not a valid English sentence. "Determines whether ..." on the other hand, would be acceptable. I think our language cop @mikes-gh should weigh in on this too.

ScarletKuro commented 2 weeks ago

If others @henon @ScarletKuro @mikes-gh can give your opinion on this that would be great.

I don't have a strong opinion on that, as it's very standard for properties and verbosity in docs doesn't scare me. Maybe lately I try to avoid word "get" for blazor parameters, because it's for the parent-child communication, and because of such https://github.com/MudBlazor/MudBlazor/issues/8485 issues the "get" might be little misleading it's not something what user expect, in the issue the user expects the parameters Hidden to be true, but it's not, because he is not using two way binding for the parent to set, therefore it doesn't really represent the real state of the component.

danielchalmers commented 2 weeks ago

"Whether the appbar will be placed at the bottom of the screen instead of the top."

Problem is, this is not a valid English sentence. "Determines whether ..." on the other hand, would be acceptable. I think our language cop @mikes-gh should weigh in on this too.

Isn't "determines whether..." as much of a fragment as "whether the..." or "the text to..."? Just more words, neither are full sentences. Doesn't seem that vital 🤷 .

henon commented 2 weeks ago

@ScarletKuro makes a very good point that sometimes you won't get the value out you expect. So maybe we should just use Sets whether ... instead of Gets or sets whether ....

henon commented 2 weeks ago

Thanks Jon!