Skip to content

Lazyloading diff-stats to improve PR loading time #29164

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

Closed
wants to merge 3 commits into from

Conversation

zokkis
Copy link
Contributor

@zokkis zokkis commented Feb 14, 2024

Lazyloading of diff-stats to improve loading time

before:
Screencast from 14.02.2024 03:15:15.webm

after:
Screencast from 14.02.2024 03:11:49.webm

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 14, 2024
@wxiaoguang
Copy link
Contributor

At first glance, it seems to be tricky and hacky. If in the future there are more and more similar approaches, I guess the code would be more and more fragile? Just my personal opinion.

@zokkis
Copy link
Contributor Author

zokkis commented Feb 14, 2024

At first glance, it seems to be tricky and hacky. If in the future there are more and more similar approaches, I guess the code would be more and more fragile? Just my personal opinion.

Is it the approche to send dependig on the htmx header, the switching Tabs with htmx in generell or just the lazyloading of the stats?

@wxiaoguang
Copy link
Contributor

In my mind, the approach could be more straightforward: introduce a new endpoint, use fetch/ajax to load the data, and render the UI (maybe also refactor the Vue component at the same time). The htmx seems an over-engineering here.

@zokkis zokkis marked this pull request as draft February 15, 2024 21:28
@wxiaoguang
Copy link
Contributor

This patch is too hacky.

I have refactored some "diff" related code, feel free to follow the new mechanism.

@wxiaoguang wxiaoguang closed this Mar 13, 2025
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants