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

Highlight viewed files differently in the PR filetree #24956

Merged
merged 5 commits into from Jun 25, 2023
Merged

Highlight viewed files differently in the PR filetree #24956

merged 5 commits into from Jun 25, 2023

Conversation

sebastian-sauer
Copy link
Contributor

image

fixes #24566

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 27, 2023
@pull-request-size pull-request-size bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 27, 2023
@delvh delvh changed the title Show IsViewed Status in filetree Highlight viewed files differently in the PR filetree May 27, 2023
delvh
delvh approved these changes May 27, 2023
View reviewed changes
web_src/js/features/pull-view-file.js Outdated Show resolved Hide resolved
@delvh delvh added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels May 27, 2023
@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 May 28, 2023
@lunny
Copy link
Member

lunny commented May 29, 2023 •
edited

Maybe the files should have a blue color to keep consistent with the checkbox color on the right column.

@silverwind
Copy link
Member

silverwind commented May 29, 2023 •
edited

Maybe the files should have a blue color to keep consistent with the checkbox color on the right column.

I would add a checkmark octicon in text color on the right side of the file name instead.

Maybe we need to also expand the width of the sidebar by 16px or more to accomodate this additional icon.

@delvh
Copy link
Member

delvh commented May 29, 2023

@silverwind see also #24566 (comment) …

@silverwind
Copy link
Member

The file icon is not completely useless because symlink should show differently. But yes, we can use it for this checkmark icon too, to conserve space.

@sebastian-sauer sebastian-sauer mentioned this pull request May 29, 2023
Closed
@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 29, 2023
silverwind
silverwind reviewed May 29, 2023
View reviewed changes
web_src/js/modules/stores.js Outdated
diffEnd: 0,
link: '',
isLoadingNewData: false,
files: []
Copy link
Member

Choose a reason for hiding this comment

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

Can files be made a Set? There can never be duplicates in the file paths. Also I think files is not a good name, maybe viewedFiles, assuming it's there only to tracked viewed files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not only a tracker of the viewed files - it's the list of all files shown in the diff.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, still could be made a Set then, to make existance checks O(1) instead of O(n).

@wxiaoguang wxiaoguang mentioned this pull request May 30, 2023
Merged
lunny pushed a commit that referenced this pull request May 30, 2023
@wxiaoguang
Refactor diffFileInfo / DiffTreeStore (#24998)
ee99cf6 
Follow  #21012, #22399

Replace #24983, fix #24938

Help #24956

Now, the `window.config.pageData.diffFileInfo` itself is a reactive
store, so it's quite easy to sync values/states by it, no need to do
"doLoadMoreFiles" or "callback".

Screenshot: these two buttons both work. After complete loading, the UI
is also right.

<details>


![image](https://github.com/go-gitea/gitea/assets/2114189/cc6310fd-7f27-45ea-ab4f-24952a87b421)


![image](https://github.com/go-gitea/gitea/assets/2114189/4c11dd67-ac03-4568-8541-91204d27a4e3)


![image](https://github.com/go-gitea/gitea/assets/2114189/38a22cec-41be-41e6-a209-f347b7a4c1de)

</details>
@silverwind
Copy link
Member

Now that #24998 is in, this needs a few adoptions.

@sebastian-sauer sebastian-sauer force-pushed the feature/24566_show_file_viewed_status_in_tree branch from 3d79f82 to d299564 Compare May 31, 2023 16:01
@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 31, 2023
wxiaoguang
wxiaoguang reviewed Jun 5, 2023
View reviewed changes
web_src/js/features/pull-view-file.js Outdated Show resolved Hide resolved
web_src/js/components/DiffFileTreeItem.vue Outdated Show resolved Hide resolved
web_src/js/components/DiffFileTreeItem.vue Outdated Show resolved Hide resolved
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Jun 24, 2023 •
edited

Ping, does it get stale?

@wxiaoguang wxiaoguang added the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Jun 24, 2023
@sebastian-sauer
Copy link
Contributor Author

Ping, does it get stale?

Sorry - will add the changes and test it now.

sebastian-sauer and others added 2 commits June 24, 2023 17:07
@sebastian-sauer
Merge remote-tracking branch 'origin/main' into feature/24566_show_fi…
4bb5e52 
…le_viewed_status_in_tree
@sebastian-sauer @wxiaoguang
Apply suggestions from code review
4727df4 
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@wxiaoguang wxiaoguang removed the issue/needs-feedback For bugs, we need more details. For features, the feature must be described in more detail label Jun 24, 2023
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 24, 2023
@sebastian-sauer
Copy link
Contributor Author

All changes applied - ready to review.

@wxiaoguang wxiaoguang added this to the 1.21.0 milestone Jun 24, 2023
@lunny lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 25, 2023
@lunny
Copy link
Member

lunny commented Jun 25, 2023

I think there is better way to indicate the viewed files on the left tree but let's merge this one and expect another PR to do that.

@lunny lunny merged commit 77e449f into go-gitea:main Jun 25, 2023
23 checks passed
@GiteaBot GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Jun 25, 2023
@delvh delvh mentioned this pull request Jul 3, 2023
Open
@sebastian-sauer sebastian-sauer deleted the feature/24566_show_file_viewed_status_in_tree branch July 9, 2023 19:36
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@silverwind silverwind silverwind left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@delvh delvh delvh approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.21.0
Development

Successfully merging this pull request may close these issues.

There are some difference between reviewed and unreviewed files on the left changed files tree.
6 participants
@sebastian-sauer @lunny @silverwind @delvh @wxiaoguang @GiteaBot