Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Display Pull Request Reviews for a User. #1545
Conversation
Need to make sure we match the start and end of the string.
We need to be able to read a user model by their login from the API.
This is needed to ensure comment summaries appear without newlines.
Displays a path and intelligently trims with ellipsis when the path doesn't fit in the allocated size.
The default expander style doesn't fit with the VS UI theme - set a new default style which uses a triangle octicon.
When the user clicks on a "Reviewer" item in the PR details view, navigate to a new view to show the PR reviews by that user.
And refactored the `PullRequestUserReviewsViewModel` a little bit to remove stuff that is no longer necessary.
Use the same triangle as used elsewhere (e.g. in `SectionControl`).
Fix a few theming issues.
|
This LGTM |
| string RelativePath { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets a comment which opens the comment in a diff view. |
jcansdale
Apr 4, 2018
Collaborator
Should this be simply, "Opens the comment in a diff view"?
Should this be simply, "Opens the comment in a diff view"?
grokys
Apr 4, 2018
Author
Contributor
Ah, should be "Gets a command...". If you look around at C# documentation, properties always start with "Gets a" (for read-only properties) or "Gets or sets a" (for read/write properties). See https://softwareengineering.stackexchange.com/questions/193244/is-the-gets-or-sets-necessary-in-xml-documentation-of-properties and http://stylecop.soyuz5.com/SA1623.html.
Ah, should be "Gets a command...". If you look around at C# documentation, properties always start with "Gets a" (for read-only properties) or "Gets or sets a" (for read/write properties). See https://softwareengineering.stackexchange.com/questions/193244/is-the-gets-or-sets-necessary-in-xml-documentation-of-properties and http://stylecop.soyuz5.com/SA1623.html.
jcansdale
Apr 4, 2018
Collaborator
Thanks, I hadn't noticed that convention.
Thanks, I hadn't noticed that convention.
| { | ||
| var text = value as string; | ||
| if (String.IsNullOrEmpty(text)) return null; | ||
| return Regex.Replace(text, @"\t|\n|\r", " "); |
jcansdale
Apr 4, 2018
Collaborator
We we want to replace tabs with a single space as well?
We we want to replace tabs with a single space as well?
grokys
Apr 4, 2018
Author
Contributor
If we're displaying a comment on a single line, it would make sense to me to convert tabs to spaces, because otherwise we'd be wasting space. Tabs are generally used for alignment between lines, so if we've got no lines it makes sense to strip them. Do you have an example where this wouldn't be a good idea?
If we're displaying a comment on a single line, it would make sense to me to convert tabs to spaces, because otherwise we'd be wasting space. Tabs are generally used for alignment between lines, so if we've got no lines it makes sense to strip them. Do you have an example where this wouldn't be a good idea?
jcansdale
Apr 4, 2018
Collaborator
Oh, I was just comparing it to the comment: "trims newlines from a string and replaces them with spaces".
Oh, I was just comparing it to the comment: "trims newlines from a string and replaces them with spaces".
|
@jcansdale addressed your feedback. |
When the user clicks on a "Reviewer" item in the PR details view, navigate to a new view to show the PR reviews by that user.
This PR also introduces:
Expander: the default WPF expander style doesn't fit in well with Visual StudioTrimmedPathTextBlockcontrol to intelligently trim paths (see screenshot above)Depends on #1523
Part of #1491