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

Improve scroll behavior when folding files on pull request "files changed" tab #23604

Closed
wants to merge 20 commits into from

Conversation

HesterG
Copy link
Contributor

@HesterG HesterG commented Mar 21, 2023

Close #23515

As described in the issue above, right now when "fold file" icon or "viewed" is clicked to fold files, it will scroll to the position before folding, resulting in confusing file jumping when scrollY is greater than certain value(approximately the offsetTop of current diff file header). This PR is to adjust scrolling behavior when folding the files (no scrolling when expanding the files).

Files can be folded in two ways: clicking viewed or clicking "fold file" icon. The folding of these two behaviors will be a little different in this PR(which can be discussed):

  1. If folded by clicking "fold file" icon and the scrollY is greater than current diff file header, scroll to header of the folded file.
  2. If folded by clicking "viewed" checkbox and the scrollY is greater than current diff file header, scroll to next file.

File folding behavior after this PR:

default.mov

Update:

Make the behavior the same in both cases(first behavior)

default.mov

@HesterG HesterG added the topic/ui Change the appearance of the Gitea UI label Mar 21, 2023
@codecov-commenter
Copy link

codecov-commenter commented Mar 21, 2023

Codecov Report

Merging #23604 (bc5c8ea) into main (f521e88) will increase coverage by 0.02%.
The diff coverage is 29.90%.

❗ Current head bc5c8ea differs from pull request most recent head 07c6ec5. Consider uploading reports for the commit 07c6ec5 to get more accurate results

@@            Coverage Diff             @@
##             main   #23604      +/-   ##
==========================================
+ Coverage   47.14%   47.17%   +0.02%     
==========================================
  Files        1149     1154       +5     
  Lines      151446   152327     +881     
==========================================
+ Hits        71397    71854     +457     
- Misses      71611    71992     +381     
- Partials     8438     8481      +43     
Impacted Files Coverage Δ
cmd/dump.go 0.67% <0.00%> (ø)
cmd/web.go 0.00% <0.00%> (ø)
models/actions/run.go 1.64% <0.00%> (-0.08%) ⬇️
models/actions/runner.go 1.44% <ø> (ø)
models/packages/package.go 45.45% <0.00%> (-1.13%) ⬇️
models/user/search.go 77.50% <0.00%> (-6.29%) ⬇️
modules/actions/github.go 0.00% <0.00%> (ø)
modules/actions/workflows.go 0.00% <0.00%> (ø)
modules/context/context.go 64.54% <0.00%> (-3.53%) ⬇️
modules/doctor/storage.go 31.93% <0.00%> (ø)
... and 28 more

... and 38 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 21, 2023
@lunny lunny added this to the 1.20.0 milestone Mar 21, 2023
@gempir
Copy link
Contributor

gempir commented Mar 21, 2023

I think behaviour 1. is great.

But 2. looks a little odd to me. I think when a user clicks on an UI element and then suddenly that UI element is gone from his view he might be confused. Ideally the filled checkbox of "viewed" would still be directly under the cursor of the user.
This costs a little more space, but gives the user feedback on his change in a way.

At least that sounds more intuitive behaviour to me, no?

@HesterG
Copy link
Contributor Author

HesterG commented Mar 21, 2023

I think behaviour 1. is great.

But 2. looks a little odd to me. I think when a user clicks on an UI element and then suddenly that UI element is gone from his view he might be confused. Ideally the filled checkbox of "viewed" would still be directly under the cursor of the user. This costs a little more space, but gives the user feedback on his change in a way.

At least that sounds more intuitive behaviour to me, no?

Yes I think so. And I think that means to make them have the same behavior (1st one). Added a commit to change that, please check the update video in description or checkout to try.

@gempir
Copy link
Contributor

gempir commented Mar 21, 2023

Don't have the time to check it out locally right now but from the video it looks great!
I think this makes a ton of sense UI wise and is easy for the user to understand.

Thank you! ❤️

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Mar 24, 2023
@wxiaoguang
Copy link
Contributor

There is an alternate ?

@HesterG HesterG closed this Apr 3, 2023
@HesterG HesterG deleted the viewedfile-23515 branch April 3, 2023 08:15
@lunny lunny removed this from the 1.20.0 milestone Apr 3, 2023
lafriks pushed a commit that referenced this pull request 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 <jimmy.praet@ksz-bcss.fgov.be>
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Aug 1, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scroll position after marking file as viewed
7 participants