Skip to content

Multiple improvements for comment edit diff #21990

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Dec 2, 2022
Merged

Multiple improvements for comment edit diff #21990

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
30dc426
Multiple improvements for comment edit diff
silverwind Nov 30, 2022
9353bc2
dropdown tweak
silverwind Nov 30, 2022
58f716c
fix double escape
silverwind Nov 30, 2022
a2a6c88
fmt
silverwind Nov 30, 2022
a285166
improve wrapping in diff
silverwind Nov 30, 2022
f9f9090
restart ci
silverwind Dec 1, 2022
a15b1f2
remove inline style, vertically align avatar
silverwind Dec 1, 2022
76ab24d
Merge branch 'main' into history-ava
lunny Dec 2, 2022
213bf66
Merge branch 'main' into history-ava
techknowlogick Dec 2, 2022
943e448
tweak triangle
silverwind Dec 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions routers/web/repo/issue_content_history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,17 @@ package repo

import (
"bytes"
"fmt"
"html"
"net/http"
"strings"

"code.gitea.io/gitea/models/avatars"
issues_model "code.gitea.io/gitea/models/issues"
"code.gitea.io/gitea/models/unit"
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/templates"
"code.gitea.io/gitea/modules/timeutil"

"github.com/sergi/go-diff/diffmatchpatch"
Expand Down Expand Up @@ -63,16 +64,20 @@ func GetContentHistoryList(ctx *context.Context) {
} else {
actionText = ctx.Locale.Tr("repo.issues.content_history.edited")
}
timeSinceText := timeutil.TimeSinceUnix(item.EditedUnix, ctx.Locale)

username := item.UserName
if setting.UI.DefaultShowFullName && strings.TrimSpace(item.UserFullName) != "" {
username = strings.TrimSpace(item.UserFullName)
}

src := html.EscapeString(item.UserAvatarLink)
class := avatars.DefaultAvatarClass + " mr-3"
name := html.EscapeString(username)
avatarHTML := string(templates.AvatarHTML(src, 28, class, username))
timeSinceText := string(timeutil.TimeSinceUnix(item.EditedUnix, ctx.Locale))

results = append(results, map[string]interface{}{
"name": fmt.Sprintf("<img class='ui avatar image' src='%s'><strong>%s</strong> %s %s",
html.EscapeString(item.UserAvatarLink), html.EscapeString(username), actionText, timeSinceText),
"name": avatarHTML + "<strong>" + name + "</strong> " + actionText + " " + timeSinceText,
"value": item.HistoryID,
})
}
Expand Down
21 changes: 9 additions & 12 deletions web_src/js/features/repo-issue-content.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,17 @@ function showContentHistoryDetail(issueBaseUrl, commentId, historyId, itemTitleH

$dialog = $(`
<div class="ui modal content-history-detail-dialog">
<i class="close icon inside"></i>
<div class="header">
${itemTitleHtml}
<div class="ui dropdown right dialog-header-options" style="display: none; margin-right: 50px;">
${i18nTextOptions} <i class="dropdown icon"></i>
${svg('octicon-x', 16, 'close icon inside')}
<div class="header df ac sb">
<div>${itemTitleHtml}</div>
<div class="ui dropdown dialog-header-options df ac mr-5 hide">
${i18nTextOptions}${svg('octicon-triangle-down', 14, 'dropdown icon')}
<div class="menu">
<div class="item red text" data-option-item="delete">${i18nTextDeleteFromHistory}</div>
</div>
</div>
</div>
<!-- ".modal .content" style was polluted in "_base.less": "&.modal > .content" -->
<div class="scrolling content" style="text-align: left; min-height: 30vh;">
Copy link
Contributor

@wxiaoguang wxiaoguang Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO The min-height: 30vh was done by purpose:

  • to show the loading UI without flicker
  • to make the dialog look better if there are only a few lines in the diff.

Copy link
Member Author

@silverwind silverwind Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could just eliminate the loading state of the modal and fetch all data after click and only show the modal when all data is present.

I don't agree about superflous whitespace on small diffs 😉

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user needs a UI feed back when they clicks the history menu item, so, something must be shown.

The dialog height in current approach can be very small, which makes the loading UI look strange.

Copy link
Member Author

@silverwind silverwind Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user needs a UI feed back when they clicks the history menu item, so, something must be shown.

Could probably add a is-loading spinner onto the clicked dropdown item. A bit unconventional but should work.

The dialog height in current approach can be very small, which makes the loading UI look strange.

Please show an example.

Copy link
Contributor

@wxiaoguang wxiaoguang Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example was there: #21990 (comment)

Or you can reproduce by:

  • open the history menu
  • disconnect your network (or emulate a laggy network)
  • click a history item

Copy link
Member Author

@silverwind silverwind Dec 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes that's the broken loading state, but if we don't have a loading state inside the modal, this won't be an issue. I'll check later.

<div class="ui loader active"></div>
</div>
<div class="comment-diff-data tl p-3 is-loading"></div>
</div>`);
$dialog.appendTo($('body'));
$dialog.find('.dialog-header-options').dropdown({
Expand Down Expand Up @@ -62,10 +59,10 @@ function showContentHistoryDetail(issueBaseUrl, commentId, historyId, itemTitleH
_csrf: csrfToken,
},
}).done((resp) => {
$dialog.find('.content').html(resp.diffHtml);
$dialog.find('.comment-diff-data').removeClass('is-loading').html(resp.diffHtml);
// there is only one option "item[data-option-item=delete]", so the dropdown can be entirely shown/hidden.
if (resp.canSoftDelete) {
$dialog.find('.dialog-header-options').show();
$dialog.find('.dialog-header-options').removeClass('hide');
}
});
},
Expand All @@ -79,7 +76,7 @@ function showContentHistoryMenu(issueBaseUrl, $item, commentId) {
const $headerLeft = $item.find('.comment-header-left');
const menuHtml = `
<div class="ui pointing dropdown top left content-history-menu" data-comment-id="${commentId}">
<a>&bull; ${i18nTextEdited} ${svg('octicon-triangle-down', 17)}</a>
&bull; <a>${i18nTextEdited} ${svg('octicon-triangle-down', 14, 'dropdown icon')}</a>
<div class="menu">
</div>
</div>`;
Expand Down
28 changes: 15 additions & 13 deletions web_src/js/svg.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import octiconChevronDown from '../../public/img/svg/octicon-chevron-down.svg';
import octiconChevronRight from '../../public/img/svg/octicon-chevron-right.svg';
import octiconCopy from '../../public/img/svg/octicon-copy.svg';
import octiconClock from '../../public/img/svg/octicon-clock.svg';
import octiconCopy from '../../public/img/svg/octicon-copy.svg';
import octiconDiffAdded from '../../public/img/svg/octicon-diff-added.svg';
import octiconDiffModified from '../../public/img/svg/octicon-diff-modified.svg';
import octiconDiffRemoved from '../../public/img/svg/octicon-diff-removed.svg';
import octiconDiffRenamed from '../../public/img/svg/octicon-diff-renamed.svg';
import octiconFile from '../../public/img/svg/octicon-file.svg';
import octiconFileDirectoryFill from '../../public/img/svg/octicon-file-directory-fill.svg';
import octiconGitMerge from '../../public/img/svg/octicon-git-merge.svg';
import octiconGitPullRequest from '../../public/img/svg/octicon-git-pull-request.svg';
Expand All @@ -20,17 +21,23 @@ import octiconProject from '../../public/img/svg/octicon-project.svg';
import octiconRepo from '../../public/img/svg/octicon-repo.svg';
import octiconRepoForked from '../../public/img/svg/octicon-repo-forked.svg';
import octiconRepoTemplate from '../../public/img/svg/octicon-repo-template.svg';
import octiconTriangleDown from '../../public/img/svg/octicon-triangle-down.svg';
import octiconFile from '../../public/img/svg/octicon-file.svg';
import octiconSidebarExpand from '../../public/img/svg/octicon-sidebar-expand.svg';
import octiconSidebarCollapse from '../../public/img/svg/octicon-sidebar-collapse.svg';
import octiconSidebarExpand from '../../public/img/svg/octicon-sidebar-expand.svg';
import octiconTriangleDown from '../../public/img/svg/octicon-triangle-down.svg';
import octiconX from '../../public/img/svg/octicon-x.svg';


export const svgs = {
'octicon-chevron-down': octiconChevronDown,
'octicon-chevron-right': octiconChevronRight,
'octicon-copy': octiconCopy,
'octicon-clock': octiconClock,
'octicon-copy': octiconCopy,
'octicon-diff-added': octiconDiffAdded,
'octicon-diff-modified': octiconDiffModified,
'octicon-diff-removed': octiconDiffRemoved,
'octicon-diff-renamed': octiconDiffRenamed,
'octicon-file': octiconFile,
'octicon-file-directory-fill': octiconFileDirectoryFill,
'octicon-git-merge': octiconGitMerge,
'octicon-git-pull-request': octiconGitPullRequest,
'octicon-issue-closed': octiconIssueClosed,
Expand All @@ -44,15 +51,10 @@ export const svgs = {
'octicon-repo': octiconRepo,
'octicon-repo-forked': octiconRepoForked,
'octicon-repo-template': octiconRepoTemplate,
'octicon-triangle-down': octiconTriangleDown,
'octicon-file': octiconFile,
'octicon-file-directory-fill': octiconFileDirectoryFill,
'octicon-sidebar-expand': octiconSidebarExpand,
'octicon-sidebar-collapse': octiconSidebarCollapse,
'octicon-diff-added': octiconDiffAdded,
'octicon-diff-modified': octiconDiffModified,
'octicon-diff-removed': octiconDiffRemoved,
'octicon-diff-renamed': octiconDiffRenamed,
'octicon-sidebar-expand': octiconSidebarExpand,
'octicon-triangle-down': octiconTriangleDown,
'octicon-x': octiconX,
};


Expand Down
18 changes: 18 additions & 0 deletions web_src/less/_repository.less
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2963,6 +2963,24 @@ tbody.commit-list {
text-align: left;
}

.comment-diff-data {
background: var(--color-code-bg);
max-height: calc(100vh - 10.5rem);
overflow-y: auto;
}

.comment-diff-data pre {
line-height: 18px;
white-space: pre-wrap;
word-break: break-all;
overflow-wrap: break-word;
}

.content-history-detail-dialog .header .avatar {
position: relative;
top: -2px;
}

#topic_edit {
margin-top: 5px;
}
Expand Down