Updated comments and parameters to reflect Microsoft documentation. #4963
Conversation
|
|
|
License agreement signed, @StephaneDelcroix - thanks for letting me know about that! |
|
I didn't correct existing comments, @StephaneDelcroix - they were the ones I added. I had the word "List" in there, when it should have been "Grid". Why would I drop the comments? |
|
I only reviewed a single commit. changing my review |
There was a problem hiding this comment.
- we do not do inline comments.
- I tend to prefer l,r,t,b over col, colspanto, ... some other team member are welcome to weight in one way or another: @hartez @samhouts @davidortinau
- I agree with you that the documentation is not helping much, and could be more descriptive and contains examples. You can propose a doc update on GitHub (https://github.com/MicrosoftDocs/xamarin-docs). Read https://docs.microsoft.com/en-us/contribute/ on how to contribute to docs.
|
Ahh, I didn't realize that. Is that in the coding spec? And, so I'm clear, you want me to delete the comments altogether? The reason I created the PR was because the parameter names are misleading. They're not about left, right, top, and bottom. They're about row, column, and spans. Am I wrong about that? If the PR is approved, I'll go through the process to update the documentation. Right now, it's effectively in sync with the code. |
both concepts are equivalent, given a little transformation. l,r,t,p indicates the position in which the item anchors itself in the grid |
|
I hear you. Still, I disagree. The two parameter signature doesn't follow that logic, or at least I feel it doesn't clearly do so. Does this mean the PR is rejected? Just want to be clear :) Thanks! |
|
y, the PR is rejected. we think the arguments are good as-is. You're more than welcome to send a PR to enhance the docs |
|
Gotcha, thanks for the heads up. Bummer - I still don't see how left, top, right and bottom make sense. I'll work on the PR for the docs - maybe I can shore those up for clarity. Do you feel that's worthwhile before I do the work? I don't want to change it if the team thinks it's fine as-is. |
|
I agree with @AuriR to rename the parameters to Column, ColumnSpan, Row and RowSpan. The left, top, right and bottom is not intuitive. |
https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.grid.igridlist-1?view=xamarin-forms
Description of Change
Short description: The parameters used when adding child Views to a Grid/IGridList are misleading, causing developer confusion.
Long description: The IGridList interface and implementation in Grid appear to have invalid parameter names. Where column and row should be, top and right are used. This is confusing to the developer. The documentation doesn't assist - it simply has Int32 in place of variable names. I've corrected these variable names as clearly as I can, and added what I feel is appropriate documentation comments for Intellisense. These are "best guess", as there is no documentation about what they definitely should be, and I don't know who to ask for verification. :)
See docs: https://docs.microsoft.com/en-us/dotnet/api/xamarin.forms.grid.igridlist-1?view=xamarin-forms
I feel the Microsoft documentation page should also be updated.
Issues Resolved
No real code changes, only parameter names, and an update to the exceptions to reflect those parameter name changes.
API Changes
None
Platforms Affected
Behavioral/Visual Changes
None
Before/After Screenshots
Not applicable
Testing Procedure
When attempting to add children to a Grid programatically, not via XAML, the Parameter List and Intellisense should now be accurate.
PR Checklist