Skip to content

Commit

Permalink
Resolve Associated Reviews and Comment Issues (#5555)
Browse files Browse the repository at this point in the history 
* Improvements to Comments and Associated Reviews

* Fix comment issues for swagger reviews
  • Loading branch information
@chidozieononiwu
chidozieononiwu authored Feb 24, 2023
1 parent 79ceced commit 40ee67f
Show file tree
Hide file tree
Showing 12 changed files with 106 additions and 89 deletions.
1 change: 1 addition & 0 deletions src/dotnet/APIView/APIViewWeb/Client/css/shared/bootstraps-overrides.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@
color: var(--base-text-color);
background-color: var(--base-fg-color);
border: 1px solid var(--border-color);
word-wrap: break-word;
}

.form-control {
Expand Down
3 changes: 2 additions & 1 deletion src/dotnet/APIView/APIViewWeb/Client/css/shared/icons.scss
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,7 +231,8 @@
.icon-comments {
cursor: pointer;
position: absolute;
right: -25px;
right: -17px;
top: 3px;
}

.icon-chevron-right {
Expand Down
18 changes: 14 additions & 4 deletions src/dotnet/APIView/APIViewWeb/Client/src/pages/review.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import Split from "split.js";
import { updatePageSettings } from "../shared/helpers";
import { updatePageSettings, toggleCommentIcon } from "../shared/helpers";
import { rightOffCanvasNavToggle } from "../shared/off-canvas";

$(() => {
Expand Down Expand Up @@ -71,14 +71,20 @@ $(() => {
if (caretDirection.endsWith("right")) {
// In case the section passed has already been replaced with more rows
if (sectionContent.length == 1) {
var sectionContentClass = sectionContent[0].className.replace(/\s/g, '.');
const sectionContentClass = sectionContent[0].className.replace(/\s/g, '.');
const sectionCommentClass = sectionContentClass.replace("code-line.", "comment-row.");
sectionContent = $(`.${sectionContentClass}`);
sectionContent.push(...$(`.${sectionCommentClass}`));
}

$.each(sectionContent, function (index, value) {
let rowClasses = $(value).attr("class");
if (rowClasses) {
if (rowClasses.match(/lvl_1_/)) { // Only show first level rows of the section
if (rowClasses.match(/lvl_1_/)) {
if (rowClasses.match(/comment-row/) && !$("#show-comments-checkbox").prop("checked")) {
toggleCommentIcon($(value).attr("data-line-id"), true);
return; // Dont show comment row if show comments setting is unchecked
}
disableCommentsOnInRowTables($(value));
$(value).removeClass("d-none");
$(value).find("svg").attr("height", `${$(value).height()}`);
Expand All @@ -88,7 +94,6 @@ $(() => {

// Update section heading icons to open state
updateSectionHeadingIcons("OPEN", caretIcon, headingRow);

}
else {
$.each(sectionContent, function (index, value) {
Expand Down Expand Up @@ -130,6 +135,11 @@ $(() => {
// Show only immediate descendants
if (startShowing) {
if (rowClasses.match(new RegExp(`lvl_${Number(subSectionLevel) + 1}_`))) {
if (rowClasses.match(/comment-row/) && !$("#show-comments-checkbox").prop("checked")) {
toggleCommentIcon($(value).attr("data-line-id"), true);
return; // Dont show comment row if show comments setting is unchecked
}

disableCommentsOnInRowTables($(value));
$(value).removeClass("d-none");
let rowHeight = $(value).height() ?? 0;
Expand Down
93 changes: 40 additions & 53 deletions src/dotnet/APIView/APIViewWeb/Client/src/shared/comments.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import { updatePageSettings } from "../shared/helpers";
import {
updatePageSettings, getCodeRow, getCodeRowSectionClasses,
getRowSectionClasses, toggleCommentIcon
} from "../shared/helpers";

$(() => {
const INVISIBLE = "invisible";
const SEL_CODE_DIAG = ".code-diagnostics";
const SEL_COMMENT_ICON = ".icon-comments";
const SEL_COMMENT_CELL = ".comment-cell";
const SHOW_COMMENTS_CHECK = "#show-comments-checkbox";
const SHOW_SYS_COMMENTS_CHECK = "#show-system-comments-checkbox";
const COMMENT_CONTENT_BOX = ".new-comment-content";
const COMMENT_TEXTBOX = ".new-thread-comment-text";

Expand All @@ -13,8 +18,6 @@ $(() => {
// simple github username match
const githubLoginTagMatch = /(\s|^)@([a-zA-Z\d-]+)/g;

let MessageIconAddedToDom = false;

$(document).on("click", ".commentable", e => {
var rowSectionClasses = getCodeRowSectionClasses(e.target.id);
showCommentBox(e.target.id, rowSectionClasses);
Expand Down Expand Up @@ -50,18 +53,16 @@ $(() => {
e.preventDefault();
});

$(document).on("click", "#show-comments-checkbox", e => {
$(document).on("click", SHOW_COMMENTS_CHECK, e => {
updatePageSettings(function () {
const checked = $("#show-comments-checkbox").prop("checked");
ensureMessageIconInDOM();
const checked = $(SHOW_COMMENTS_CHECK).prop("checked");
toggleAllCommentsVisibility(checked);
});
});

$(document).on("click", "#show-system-comments-checkbox", e => {
$(document).on("click", SHOW_SYS_COMMENTS_CHECK, e => {
updatePageSettings(function () {
const checked = $("#show-system-comments-checkbox").prop("checked");
ensureMessageIconInDOM();
const checked = $(SHOW_SYS_COMMENTS_CHECK).prop("checked");
toggleAllDiagnosticsVisibility(checked);
});
});
Expand Down Expand Up @@ -327,6 +328,16 @@ $(() => {
$(document).ready(function() {
highlightCurrentRow();
addCommentThreadNavigation();
$(SEL_COMMENT_CELL).each(function () {
const id = getElementId(this);
const checked = $(SHOW_COMMENTS_CHECK).prop("checked");
toggleCommentIcon(id, !checked);
});
$(SEL_CODE_DIAG).each(function () {
const id = getElementId(this);
const checked = $(SHOW_SYS_COMMENTS_CHECK).prop("checked");
toggleCommentIcon(id, !checked);
});
});

$(document).on("click", ".comment-group-anchor-link", e => {
Expand All @@ -352,8 +363,6 @@ $(() => {
});
}



function getReviewId(element: HTMLElement) {
return getParentData(element, "data-review-id");
}
Expand Down Expand Up @@ -385,25 +394,6 @@ $(() => {
return $(sibling).prevAll("a").first().find("small");
}

function getCodeRowSectionClasses(id: string) {
var codeRow = getCodeRow(id);
var rowSectionClasses = "";
if (codeRow) {
rowSectionClasses = getRowSectionClasses(codeRow[0].classList);
}
return rowSectionClasses;
}

function getRowSectionClasses(classList: DOMTokenList) {
const rowSectionClasses: string[] = [];
for (const value of classList.values()) {
if (value == "section-loaded" || value.startsWith("code-line-section-content") || value.match(/lvl_[0-9]+_(parent|child)_[0-9]+/)) {
rowSectionClasses.push(value);
}
}
return rowSectionClasses.join(' ');
}

function getCommentId(element: HTMLElement) {
return getParentData(element, "data-comment-id");
}
Expand Down Expand Up @@ -431,10 +421,6 @@ $(() => {
return $(`.comment-row[data-line-id='${id}']`);
}

function getCodeRow(id: string) {
return $(`.code-line[data-line-id='${id}']`);
}

function getDiagnosticsRow(id: string) {
return $(`.code-diagnostics[data-line-id='${id}']`);
}
Expand Down Expand Up @@ -552,38 +538,39 @@ $(() => {

function toggleAllCommentsVisibility(showComments: boolean) {
$(SEL_COMMENT_CELL).each(function () {
var id = getElementId(this);
const id = getElementId(this);
if (id) {
getCommentsRow(id).toggle(showComments);
toggleCommentIcon(id, !showComments);
const tbRow = getCommentsRow(id);
const prevRow = tbRow.prev(".code-line");
const nextRow = tbRow.next(".code-line");
if ((prevRow != undefined && prevRow.hasClass("d-none")) && (nextRow != undefined && nextRow.hasClass("d-none")))
return;

(showComments) ? tbRow.removeClass("d-none") : tbRow.addClass("d-none");
toggleCommentIcon(id, !showComments);
}
});
}

function toggleAllDiagnosticsVisibility(showComments: boolean) {
$(SEL_CODE_DIAG).each(function () {
var id = getElementId(this);
const id = getElementId(this);
if (id) {
getDiagnosticsRow(id).toggle(showComments);
toggleCommentIcon(id, !showComments);
const tbRow = getDiagnosticsRow(id);
const prevRow = tbRow.prev(".code-line");
const nextRow = tbRow.next(".code-line");
if ((prevRow != undefined && prevRow.hasClass("d-none")) && (nextRow != undefined && nextRow.hasClass("d-none")))
return;

(showComments) ? tbRow.removeClass("d-none") : tbRow.addClass("d-none");
toggleCommentIcon(id, !showComments);
}
});
}

function toggleSingleCommentAndDiagnostics(id: string) {
getCommentsRow(id).toggle();
getDiagnosticsRow(id).toggle();
}

function ensureMessageIconInDOM() {
if (!MessageIconAddedToDom) {
$(".comment-icon-cell").append(`<span class="icon icon-comments ` + INVISIBLE + `"><i class="far fa-comment-alt pt-1 pl-1"></i></span>`);
MessageIconAddedToDom = true;
}
}

function toggleCommentIcon(id, show: boolean) {
getCodeRow(id).find(SEL_COMMENT_ICON).toggleClass(INVISIBLE, !show);
getCommentsRow(id).toggleClass("d-none");
getDiagnosticsRow(id).toggleClass("d-none");
}

function getDisplayedCommentRows(commentRows: JQuery<HTMLElement>, clearCommentAnchors = false, returnFirst = false) {
Expand Down
27 changes: 27 additions & 0 deletions src/dotnet/APIView/APIViewWeb/Client/src/shared/helpers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,30 @@ export function updatePageSettings(callBack) {
url: uri
}).done(callBack());
}

export function getCodeRow(id: string) {
return $(`.code-line[data-line-id='${id}']`);
}

export function getCodeRowSectionClasses(id: string) {
var codeRow = getCodeRow(id);
var rowSectionClasses = "";
if (codeRow) {
rowSectionClasses = getRowSectionClasses(codeRow[0].classList);
}
return rowSectionClasses;
}

export function getRowSectionClasses(classList: DOMTokenList) {
const rowSectionClasses: string[] = [];
for (const value of classList.values()) {
if (value == "section-loaded" || value.startsWith("code-line-section-content") || value.match(/lvl_[0-9]+_(parent|child)_[0-9]+/)) {
rowSectionClasses.push(value);
}
}
return rowSectionClasses.join(' ');
}

export function toggleCommentIcon(id, show: boolean) {
getCodeRow(id).find(".icon-comments").toggleClass("invisible", !show);
}
2 changes: 1 addition & 1 deletion src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Index.cshtml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
<div class="row mx-5 py-2">
<div class="col-md-10 px-0 d-sm-flex flex-row justify-content-sm-start">
<div class="input-group" id="reviews-table-search-container">
<input type="search" class="form-control" id="reviews-table-search-box" placeholder="Search.." />
<input type="search" class="form-control" id="reviews-table-search-box" placeholder="Search.. e.g pr:<Pull-Request Number>, package:<Package Name>, author:<Github UserName>" />
<button class="btn btn-outline-primary" id="reviews-search-button" aria-labelledby="reviews-table-search-box" type="button"><i class="fa-solid fa-magnifying-glass"></i></button>
</div>
</div>
Expand Down
7 changes: 3 additions & 4 deletions src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Review.cshtml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,8 +229,7 @@
</ul>
}
}
@if ((Model.Review.Language == "Swagger" || Model.Review.Language == "Cadl") &&
Model.Review.FilterType == ReviewType.PullRequest)
@if (Model.Review.FilterType == ReviewType.PullRequest)
{
var prsOfAssociatedReviews = await Model.GetPRsOfAssoicatedReviews();
@if (prsOfAssociatedReviews != null && prsOfAssociatedReviews.Count() > 1)
Expand All @@ -254,7 +253,7 @@
id = pr.ReviewId,
});
<li class="list-group-item">
<a href="@url" target="_blank">@(pr.Language ?? pr.ReviewId)</a>
<a href="@url" target="_blank">@pr.Language/@pr.PackageName</a>
</li>
}
}
Expand All @@ -268,7 +267,7 @@
var reviewOptionsCollapseState = " show";
if (Request.Cookies.ContainsKey("reviewOptionsCollapse"))
{
if (Request.Cookies["reviewOptionsCollapse"].Equals("shown"))
if (!Request.Cookies["reviewOptionsCollapse"].Equals("shown"))
reviewOptionsCollapseState = String.Empty;
}
}
Expand Down
18 changes: 2 additions & 16 deletions src/dotnet/APIView/APIViewWeb/Pages/Assemblies/Review.cshtml.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,7 @@ public async Task<PartialViewResult> OnGetCodeLineSectionAsync(
var renderedCodeFile = await _codeFileRepository.GetCodeFileAsync(Revision);
var fileDiagnostics = renderedCodeFile.CodeFile.Diagnostics ?? Array.Empty<CodeDiagnostic>();
CodeLine[] currentHtmlLines;
var userPrefernce = await _preferenceCache.GetUserPreferences(User) ?? new UserPreferenceModel();

if (DiffRevision != null)
{
Expand Down Expand Up @@ -226,7 +227,7 @@ public async Task<PartialViewResult> OnGetCodeLineSectionAsync(
Lines = CreateLines(fileDiagnostics, currentHtmlLines, Comments, true);
}
TempData["CodeLineSection"] = Lines;
TempData["UserPreference"] = PageModelHelpers.GetUserPreference(_preferenceCache, User) ?? new UserPreferenceModel();
TempData["UserPreference"] = userPrefernce;
return Partial("_CodeLinePartial", sectionKey);
}

Expand Down Expand Up @@ -256,16 +257,6 @@ public async Task<ActionResult> OnPostRequestReviewersAsync(string id, HashSet<s
return RedirectToPage(new { id = id });
}

public IActionResult OnGetUpdatePageSettings(bool hideLineNumbers = false, bool hideLeftNavigation = false)
{
_preferenceCache.UpdateUserPreference(new UserPreferenceModel()
{
HideLeftNavigation = hideLeftNavigation,
HideLineNumbers = hideLineNumbers
}, User);
return new EmptyResult();
}

public Dictionary<string, string> GetRoutingData(string diffRevisionId = null, bool? showDiffOnly = null, bool? showDocumentation = null, string revisionId = null)
{
var routingData = new Dictionary<string, string>();
Expand All @@ -276,11 +267,6 @@ public Dictionary<string, string> GetRoutingData(string diffRevisionId = null, b
return routingData;
}

public UserPreferenceModel GetUserPreference()
{
return _preferenceCache.GetUserPreferences(User).Result;
}

public async Task<IEnumerable<PullRequestModel>> GetAssociatedPullRequest()
{
return await _pullRequestManager.GetPullRequestsModel(Review.ReviewId);
Expand Down
4 changes: 4 additions & 0 deletions src/dotnet/APIView/APIViewWeb/Pages/Assemblies/_CodeLine.cshtml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,7 @@
{
<a class="line-comment-button invisible">+</a>// Added for visual consistency
}
<span class="icon icon-comments invisible"><i class="far fa-comment-alt pt-1 pl-1"></i></span>
</td>
<td class="line-details-button-cell">
@if(Model.DocumentedByLines?.Length > 0)
Expand Down Expand Up @@ -226,6 +227,9 @@
var warningDiags = Model.Diagnostics.Where(d => d.Level == APIView.CodeDiagnosticLevel.Warning);
var infoDiags = Model.Diagnostics.Where(d => d.Level == APIView.CodeDiagnosticLevel.Info);
var diagnosticsClass = "code-diagnostics";
if (userPreference.ShowSystemComments != true)
diagnosticsClass += " d-none";

if (Model.CodeLine.IsHiddenApi)
{
diagnosticsClass += PageModelHelpers.GetHiddenApiClass(userPreference);
Expand Down
9 changes: 1 addition & 8 deletions src/dotnet/APIView/APIViewWeb/Pages/Shared/_CodeLinePartial.cshtml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,7 @@
<td class="line-details">
<table>
<tr>
@if(userPreference.HideLineNumbers == true)
{
<td class="line-number"><span></span></td>
}
else
{
<td class="line-number"><span></span></td>
}
<td class="line-number"><span></span></td>
<td class="line-details-button-cell">
<a class="line-comment-button" style="visibility: hidden;">+</a> <!-- Added for visual consistency -->
</td>
Expand Down
Loading

0 comments on commit 40ee67f

Please sign in to comment.