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.

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

Scroll position after marking file as viewed #23515

Closed
gempir opened this issue Mar 16, 2023 · 1 comment
Closed

Scroll position after marking file as viewed #23515

gempir opened this issue Mar 16, 2023 · 1 comment
Assignees
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@gempir
Copy link
Contributor

gempir commented Mar 16, 2023

Feature Description

When you are reviewing a pull request, it's pretty common in our team to use the great feature of marking files as "viewed".

When you click viewed the file folds closed. When you have large diffs they have a sticky header and you can still click "viewed" even though you are at the bottom of the file, great!

The issue now:
When you mark a file as viewed while at the end of the big file, your scroll position ends up weird because the page got smaller. Now you might have skipped a few files or part of a diff and need to scroll back up to the "next" file.

Possible solution:
My idea was just scrolling the view to the folded file at the moment you mark the file as "viewed".
Then you would know where you are on the page.

Other suggestions are of course welcome.

In the video below I'm reviewing the docker-compose.yml and mark it as viewed. I expect the next file for me to review to be status.go (because it's the next file vertically). But I land in the middle of a main.go file.

Screenshots

pr_jump.mov
@gempir gempir added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels Mar 16, 2023
@HesterG HesterG self-assigned this Mar 20, 2023
lafriks pushed a commit that referenced this issue Apr 5, 2023
Backport #23702 by @jpraet

Fixes #23701, #23515.

Alternate approach to #23604 using CSS scroll-margin-top, which is also
taken into account for direct links to files in a diff:

* On the PR diff, this currently shows the previous file first:
https://try.gitea.io/jpraet/test/pulls/13/files#diff-b94d08b409f9d05fb65b6cccaf7b3e4ecc7cc333
* On the commit diff, the first line of the linked file is currently
under the sticky header:
https://try.gitea.io/jpraet/test/commit/1a19e6b14e31d295b7372f3346580c3e85690ff5#diff-b94d08b409f9d05fb65b6cccaf7b3e4ecc7cc333

Co-authored-by: Jimmy Praet <[email protected]>
@delvh
Copy link
Member

delvh commented Apr 24, 2023

Closed by #23702?

@delvh delvh closed this as completed Apr 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants