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 implementation of diff-file-tree #32768

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

kerwin612
Copy link
Member

@kerwin612 kerwin612 commented Dec 9, 2024

before:
before
The content on the right is rendered by the backend, while the content on the left is rendered by the frontend. Under slow network conditions, the visual experience becomes significantly disjointed.

after:
after
The content on the left has been changed to be rendered by the backend, the same as the right.

The rendering of the file tree has been changed from Vue to Go templates, enhancing the visual experience.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 9, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 9, 2024
@github-actions github-actions bot added modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files modifies/frontend labels Dec 9, 2024
@kerwin612 kerwin612 marked this pull request as draft December 9, 2024 06:41
@kerwin612 kerwin612 marked this pull request as ready for review December 9, 2024 07:01
@wxiaoguang

This comment was marked as outdated.

@kerwin612
Copy link
Member Author

Firstly, we need to reach a consensus on whether the issue I've raised (inconsistent rendering methods for left and right content, resulting in a visually disjointed experience under slow network conditions) is a problem that needs to be optimized and solved.

If this issue needs to be fixed, I personally believe the best approach would be: ① to have the initial data rendering completed by the backend, just like the content on the right side; ② to leave the page interaction logic to the frontend, such as showing/hiding the file tree, selecting/collapsing/expanding tree nodes, etc. (This PR is implemented according to this logic.)

otherwise it's very difficult to maintain these 2 different "components" at the same time, it is fragile and difficult to test, they will be definitely break one day.

The implementation of this PR has already removed the dependency on Vue components; therefore, in principle, the Vue components can be deleted.

The current logic is very clear: all initial data rendering is handled by the Go template; only a small amount of page interaction logic is handled by JavaScript. It's straightforward and well-defined. I don't see any mixing here. Isn't this logic prevalent in most of the code in the system?

@wxiaoguang I sincerely appreciate your insights and am eagerly taking them to heart. Thank you for your valuable guidance.

@wxiaoguang
Copy link
Contributor

This improvement is trivial in my mind because it doesn't really bother me, and the visual feedback could be improved by some other simple approaches (eg: adding a "loading" indicator).

Since I can see there are a lot of high priority TODOs in Gitea (eg: my 32779, 32788, etc), I guess I don't have time to take a deep look into it at the moment.

@kerwin612
Copy link
Member Author

This commit e12f09c, simply adds a loading animation. Preview as follows:
Normal network:
1-before-1
Slow network:
1-before-2

@wxiaoguang
Copy link
Contributor

Maybe you could take a look at how GitHub implements the "file tree" with a lot of files.

Key points:

  1. frontend rendering
  2. webcomponent ("loading indicator" for first time, then JS code cache could make it fast next time)

https://github.com/google/material-design-icons/tree/master/symbols/ios/10k

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/frontend modifies/go Pull requests that update Go code modifies/templates This PR modifies the template files size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants