Skip to content
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

Jump to bottom

Imrove scroll behavior to hash issuecomment(scroll position, auto expand if file is folded, and on refreshing) #23513

Merged
merged 19 commits into from
Mar 17, 2023
Merged

Imrove scroll behavior to hash issuecomment(scroll position, auto expand if file is folded, and on refreshing) #23513

Changes from 16 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
f5ec800
scroll to viewed comment
HesterG Mar 15, 2023
9d4b530
Merge branch 'go-gitea:main' into viewed-file-scroll
HesterG Mar 15, 2023
a8a1d57
Merge branch 'go-gitea:main' into viewed-file-scroll
HesterG Mar 16, 2023
08b8c0c
forget pos remember when having hash
HesterG Mar 16, 2023
63892e8
add condition
HesterG Mar 16, 2023
40dc855
add comments
HesterG Mar 16, 2023
8feda9d
update comment
HesterG Mar 16, 2023
43113ee
fix lint
HesterG Mar 16, 2023
e0c9898
fix lint
HesterG Mar 16, 2023
a46b186
Merge branch 'main' into viewed-file-scroll
HesterG Mar 16, 2023
7bea4fc
use scrolltop
HesterG Mar 16, 2023
21b02fa
use scrolltop
HesterG Mar 16, 2023
323b10d
Merge branch 'main' into viewed-file-scroll
HesterG Mar 16, 2023
5aef97a
use height instead of hard code
HesterG Mar 16, 2023
6e20865
use jquery
HesterG Mar 16, 2023
32d3eea
use outerheight
HesterG Mar 16, 2023
522fa67
Merge branch 'main' into viewed-file-scroll
HesterG Mar 17, 2023
59e98c9
use window.scrollto
HesterG Mar 17, 2023
96f267d
Merge branch 'main' into viewed-file-scroll
HesterG Mar 17, 2023
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
19 changes: 18 additions & 1 deletion web_src/js/features/repo-issue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {initEasyMDEImagePaste} from './comp/ImagePaste.js';
import {initCompMarkupContentPreviewTab} from './comp/MarkupContentPreview.js';
import {initTooltip, showTemporaryTooltip} from '../modules/tippy.js';
import {hideElem, showElem, toggleElem} from '../utils/dom.js';
import {setFileFolding} from './file-fold.js';

const {appSubUrl, csrfToken} = window.config;

Expand Down Expand Up @@ -437,17 +438,33 @@ export async function handleReply($el) {

export function initRepoPullRequestReview() {
if (window.location.hash && window.location.hash.startsWith('#issuecomment-')) {
// set scrollRestoration to 'manual' when there is a hash in url, so that the scroll position will not be remembered after refreshing
if (window.history.scrollRestoration !== 'manual') {
window.history.scrollRestoration = 'manual';
}
const commentDiv = $(window.location.hash);
if (commentDiv) {
// get the name of the parent id
const groupID = commentDiv.closest('div[id^="code-comments-"]').attr('id');
if (groupID && groupID.startsWith('code-comments-')) {
const id = groupID.slice(14);
const ancestorDiffBox = commentDiv.closest('.diff-file-box');
// on pages like conversation, there is no diff header
const diffHeader = ancestorDiffBox.find('.diff-file-header');
// offset is for scrolling
let offset = 30;
if (diffHeader[0]) {
offset += $('.diff-detail-box').outerHeight() + diffHeader.outerHeight();
}
$(`#show-outdated-${id}`).addClass('gt-hidden');
$(`#code-comments-${id}`).removeClass('gt-hidden');
$(`#code-preview-${id}`).removeClass('gt-hidden');
$(`#hide-outdated-${id}`).removeClass('gt-hidden');
commentDiv[0].scrollIntoView();
// if the comment box is folded, expand it
if (ancestorDiffBox.attr('data-folded') && ancestorDiffBox.attr('data-folded') === 'true') {
setFileFolding(ancestorDiffBox[0], ancestorDiffBox.find('.fold-file')[0], false);
}
$('html, body').scrollTop(commentDiv.offset().top - offset);
Copy link
Contributor

@wxiaoguang wxiaoguang Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Why two selectors? The scrollTop should be invoked on a clear target IMO.

ps: I just thought it's a good chance to rewrite all the legacy jQuery code to Vanilla JS 😂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/12222485/why-to-use-html-body-for-scrolltop-instead-of-just-html

For the two selectors, I saw this. I think it might be a good idea as we are also using vue, and it sometimes feels strange to have jquery at the same time. But I think I still lack an overall view to the product to comment on this right now..

Copy link
Contributor

Choose a reason for hiding this comment

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

But that's too old, 10 years ago ... I do not think Gitea supports so old browsers. What's the modern and proper approach, by MDN?

Copy link
Member

@silverwind silverwind Mar 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think html aka document.documentElement is sufficient, tested on try.gitea.io:

image

So if you want to scroll the page in JS to a certain offset:

document.documentElement.scrollTop = value

Thought I generally recommend Element.scrollIntoView() instead of scrolling to calculated offsets, if that is an option here, but I think it isn't. Or even better, let browser-native anchor scrolling take effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep scrollIntoView cannot set offset value, added a commit to use window.scrollto

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good as well.

}
}
}
Expand Down