Improve reviewing PR UX #19612

Gusted · 2022-05-04T23:40:01Z

When you're reviewing a PR, a counter of all pending comments will be shown inline within the review box. This can help by making the decision if the amount of comments is good enough for a first round review etc.
When adding a new comment, the reviewbox will be pulsed. This is to indicate that these comments are still pending and acts as little hint to newcomers that it's intended to go trough this process(To approve/comment/request changes).
Resolves Pulse the Review button when a comment is added to a review #19611

Screenshots

See the counter of pending comments:

Animation of pulse:
https://user-images.githubusercontent.com/25481501/166842189-73785fef-d3ac-45a5-8bd9-bec02a008aea.mp4

wxiaoguang

I do not think mixing jQuery and native JS together is a good idea.

And the native code is so long, the code itself is wordy and the comments are also wordy .....

silverwind · 2022-05-05T08:28:03Z

And the native code is so long

It's just a matter of getting used to it. It'll still perform much better than any jQuery-equivalent code.

web_src/js/features/repo-issue.js

web_src/js/features/repo-diff.js

web_src/js/features/repo-issue.js

routers/web/repo/pull.go

wxiaoguang · 2022-05-05T09:22:31Z

It's just a matter of getting used to it. It'll still perform much better than any jQuery-equivalent code.

Does the performance really matter or can you feel it? C perform better than Go , WASM also does better performance, do they make sense to be used?

delvh · 2022-05-05T11:22:01Z

@wxiaoguang I'd say it is a tradeoff of the benefit versus the associated cost:
On the question Go vs C, C is perhaps 5% faster but needs potentially massive changes.
In JQuery, as far as I have heard, performance can be increased by a factor of up to 10 by using the native API.
And unlike the Go to C conversion, all that must be changed is expanding the current query calls with maybe thirty more chars (per function call).

Additionally, the documentation of the native DOM API is quite extensive and regularly updated (i.e. MDN), while the documentation of JQuery can be quite sparse (for example, when I searched for the meaning of $(), I couldn't find anything and had to look at their source code).
So the native API is definitely more suited for frontend newcomers (like me), who have knowledge in neither of the two.

Also, if we would manage to get rid of JQuery, the total frontend download size might decrease, and our frontend will have one fewer possible point of failure (i.e. the JQuery mirror going offline for some reason)

But this whole discussion is irrelevant for this PR, and I think we both know how we think about this issue.

wxiaoguang · 2022-05-05T11:25:58Z

There have been a lot of discussions before.

In JQuery, as far as I have heard, performance can be increased by a factor of up to 10 by using the native API.

Well, you just heard of it. Can you show how it makes Gitea UI better without jQuery? Or how jQuery makes Gitea UI slow?

while the documentation of JQuery can be quite sparse

How could it be spare? Isn't it on https://api.jquery.com and written very clearly? Won't you still read the MDN if you write native JS?

if we would manage to get rid of JQuery, the total frontend download size might decrease ....

My question is: since Fomantic UI still exists, how can you get rid of jQuery? If you can not get rid of it, why you want to mix the jQuery code with native JS code together? Any benefit?

My belief is: show real evidences, make feasible plans, write the executable and clear code, instead of guessing and imagining.

silverwind · 2022-05-05T11:37:49Z

Let's not diverge again into the jQuery vs native discussion. Both approaches are fine currently, and I personally have no issues with mixing them either.

wxiaoguang · 2022-05-05T11:38:30Z

Let's not diverge again into the jQuery vs native discussion. Both approaches are fine currently, and I personally have no issues with mixing them either.

This PR DOES mix them together. A lot of native code are added into the jQuery code context. And I would against for the re-inventing wheels behavior. (Why you want to write the getCSSVariable instead of using $.css directly?)

If you want a new PR wihout any jQuery context (for example, a separate Vue component), I would say nothing.

wxiaoguang · 2022-05-05T11:50:38Z

And we have written guildelines. If the guideline is incorrect, please improve it to keep Gitea code base clear and maintainable.

I never want to see a 4000 line index.js with mixed native/jQuery/Vue together again.

https://docs.gitea.io/en-us/guidelines-frontend/

silverwind · 2022-05-05T11:54:56Z

Why you want to write the getCSSVariable instead of using $.css directly

Often times it's beneficial from a DRY point of view to variablize CSS values so they can be shared for other rules, but in this specific case, there's not much use to that as it's a very specific duration.

Co-authored-by: silverwind <[email protected]>

Gusted · 2022-05-05T22:42:50Z

I don't mind refactoring this file to remove jQuery. I can only write native javascript(especially when trying out different approaches to a problem). jQuery would still require me to lookup docs/current code in Gitea to implement it correctly. If it's a really must, I don't mind changing my current code to jQuery. But then this approach to implement this feature should be settled before then.

wxiaoguang · 2022-05-06T02:23:54Z

You are in a jQuery context, and you are using jQuery, why not keep using it?

As I said before, I would against for mixing different frameworks together, it only make code messy and unmaintainable.

If a JS module is written from scratch without Fomantic UI & jQuery dependency, then it's free to use its own framework.

If a JS feature is written inside jQuery context, I can not imagine a reason why not keep using jQuery.

wxiaoguang · 2022-05-06T02:28:37Z

routers/web/repo/pull.go

+	ctx.Data["PendingCodeComments"] = numPendingCodeComments
+	ctx.PageData["prReview"] = map[string]interface{}{
+		"pendingCodeComments": numPendingCodeComments,
+	}


Hmm ... it's duplicated. I think we can use data-xxx to pass the template data to JS in this case.

wxiaoguang · 2022-05-06T03:24:16Z

I made a refactoring demo, you can take what you like.

Refactor (demo) Gusted/gitea#3

Some thoughts:

Setting numPendingCodeComments twice to Data and PageData is redundant. Only setting template Data is enough.
You can use data-pending-comment-number="0" css selector to hide the element automatically.
The e.target.closest('form') has been executed above so the result can be used directly, instead of duplicate call (bad performance)
The setTimeout doesn't make sense
jQuery can also do reflow easily

And there are still some bugs, the counter number could be incorrect in some cases. For example, if the network is broken before form submit, you would see the number go high incorrectly (to the moon). This problem is caused by some more fundamental problems in Gitea frontend (messy logic, bad designs, bad codes), so it's not a blocker for this PR.

Just because there were so many bad designs and bad codes before, I spent a lot of time refactoring the frontend code. So what I am doing now is trying to make Gitea have a better frontend code base and benefits all developers in the future.

And I really hope everyone (especially people with frontend experiences) can work together to make Gitea frontend better.

ps: Although you said "can only write native javascript & need to look up docs for jQuery)", that's true for anyone about "I can only write A & need to look up docs for B" (A=Golang, B=JavaScript, etc). jQuery is simple enough and it won't waste you more than 30 minutes to understand how it works. Since Gitea was and is using jQuery heavily, I think you should know and learn it, just like you should learn your enemy before any fight.

Gusted · 2022-05-06T16:58:19Z

ps: Although you said "can only write native javascript & need to look up docs for jQuery)", that's true for anyone about "I can only write A & need to look up docs for B" (A=Golang, B=JavaScript, etc). jQuery is simple enough and it won't waste you more than 30 minutes to understand how it works. Since Gitea was and is using jQuery heavily, I think you should know and learn it, just like you should learn your enemy before any fight.

I write jQuery on a monthly basis, much of the specifics will be forgotten by then. The 30 minutes of looking everything up is more or less a repeated task every time I want to convert my javascript code to jQuery code.

zeripath · 2022-05-06T19:09:34Z

make lgtm work

wxiaoguang · 2022-05-07T05:39:22Z

This PR has done what it should do, it's enough.

Some hints about how to make the comment counting part better (for future readers)

The counting number should be returned by backend for every request, then the UI part shouldn't do the counting. Only backend's counting number is correct. Frontend counting will be out-of-sync in some cases.
Some event handlers seem to be installed more than once. That's the reason why the number go high incorrectly (to the moon) in some cases, and it would case more problems (eg: duplicate requests). In future, the frontend event handlers should be made as added once only.

* giteaofficial/main: Delete related PullAutoMerge and ReviewState on User/Repo Deletion (go-gitea#19649) Allow custom default merge message with .gitea/default_merge_message/<merge_style>_TEMPLATE.md (go-gitea#18177) Allow to mark files in a PR as viewed (go-gitea#19007) Auto merge pull requests when all checks succeeded via API (go-gitea#9307) Hide private repositories in packages (go-gitea#19584) Only show accessible teams in dashboard dropdown list (go-gitea#19642) prevent double click new issue/pull/comment button (go-gitea#16157) Improve reviewing PR UX (go-gitea#19612) [skip ci] Updated translations via Crowdin Add Changelog v1.16.7 (go-gitea#19575) (go-gitea#19644) Set safe dir for git operations in .drone.yml CI (go-gitea#19641) Add missing `sorting` column in `project_issue` table (go-gitea#19635)

wxiaoguang · 2024-11-21T12:26:36Z

This PR has done what it should do, it's enough.

Some hints about how to make the comment counting part better (for future readers)

1. The counting number should be returned by backend for every request, then the UI part shouldn't do the counting. Only backend's counting number is correct. Frontend counting will be out-of-sync in some cases.

2. Some event handlers seem to be installed more than once. That's the reason why `the number go high incorrectly (to the moon)` in some cases, and it would case more problems (eg: duplicate requests). In future, the frontend event handlers should be made as added once only.

To fixes these longstanding bugs: Fix PR diff review form submit #32596

Gusted added 2 commits May 5, 2022 00:14

Add pulse

6495e6d

Add number of pending comments

e9c9179

Gusted added this to the 1.17.0 milestone May 4, 2022

Gusted added type/feature Completely new functionality. Can only be merged if feature freeze is not active. topic/ui-interaction Change the process how users use Gitea instead of the visual appearance labels May 4, 2022

Merge branch 'main' into improve-ux-reviewing

44b60d3

This comment was marked as off-topic.

Sign in to view

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 5, 2022

wxiaoguang reviewed May 5, 2022

View reviewed changes

silverwind reviewed May 5, 2022

View reviewed changes

web_src/js/features/repo-issue.js Outdated Show resolved Hide resolved

silverwind reviewed May 5, 2022

View reviewed changes

web_src/js/features/repo-diff.js Outdated Show resolved Hide resolved

silverwind reviewed May 5, 2022

View reviewed changes

web_src/js/features/repo-diff.js Outdated Show resolved Hide resolved

delvh reviewed May 5, 2022

View reviewed changes

web_src/js/features/repo-diff.js Outdated Show resolved Hide resolved

web_src/js/features/repo-diff.js Outdated Show resolved Hide resolved

web_src/js/features/repo-issue.js Outdated Show resolved Hide resolved

routers/web/repo/pull.go Outdated Show resolved Hide resolved

Gusted and others added 4 commits May 5, 2022 22:23

Use string type

5946f3f

Co-authored-by: silverwind <[email protected]>

Use string type

c6cf831

Co-authored-by: silverwind <[email protected]>

Remove class before adding it again.

f2e8d3a

Make linter happy

f7f9138

Gusted added 2 commits May 6, 2022 00:52

Use PageData

feef2d6

Merge branch 'main' into improve-ux-reviewing

702c5bd

delvh approved these changes May 5, 2022

View reviewed changes

GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 5, 2022

GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 5, 2022

wxiaoguang reviewed May 6, 2022

View reviewed changes

Refactor (#3)

27cdfcc

Merge branch 'main' into improve-ux-reviewing

ef484f2

zeripath approved these changes May 6, 2022

View reviewed changes

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 May 6, 2022

Merge branch 'main' into improve-ux-reviewing

dd93fb4

Merge branch 'main' into improve-ux-reviewing

d5c126e

techknowlogick merged commit 0eac09e into go-gitea:main May 7, 2022

AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022

Improve reviewing PR UX (go-gitea#19612)

f2962da

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve reviewing PR UX #19612

Improve reviewing PR UX #19612

Gusted commented May 4, 2022

This comment was marked as off-topic.

wxiaoguang left a comment

silverwind commented May 5, 2022

wxiaoguang commented May 5, 2022

delvh commented May 5, 2022 •

edited

Loading

wxiaoguang commented May 5, 2022 •

edited

Loading

silverwind commented May 5, 2022

wxiaoguang commented May 5, 2022 •

edited

Loading

wxiaoguang commented May 5, 2022

silverwind commented May 5, 2022 •

edited

Loading

Gusted commented May 5, 2022

wxiaoguang commented May 6, 2022

wxiaoguang May 6, 2022

wxiaoguang commented May 6, 2022 •

edited

Loading

Gusted commented May 6, 2022 •

edited

Loading

zeripath commented May 6, 2022

wxiaoguang commented May 7, 2022

wxiaoguang commented Nov 21, 2024

Improve reviewing PR UX #19612

Improve reviewing PR UX #19612

Conversation

Gusted commented May 4, 2022

Screenshots

This comment was marked as off-topic.

wxiaoguang left a comment

Choose a reason for hiding this comment

silverwind commented May 5, 2022

wxiaoguang commented May 5, 2022

delvh commented May 5, 2022 • edited Loading

wxiaoguang commented May 5, 2022 • edited Loading

silverwind commented May 5, 2022

wxiaoguang commented May 5, 2022 • edited Loading

wxiaoguang commented May 5, 2022

silverwind commented May 5, 2022 • edited Loading

Gusted commented May 5, 2022

wxiaoguang commented May 6, 2022

wxiaoguang May 6, 2022

Choose a reason for hiding this comment

wxiaoguang commented May 6, 2022 • edited Loading

Gusted commented May 6, 2022 • edited Loading

zeripath commented May 6, 2022

wxiaoguang commented May 7, 2022

wxiaoguang commented Nov 21, 2024

delvh commented May 5, 2022 •

edited

Loading

wxiaoguang commented May 5, 2022 •

edited

Loading

wxiaoguang commented May 5, 2022 •

edited

Loading

silverwind commented May 5, 2022 •

edited

Loading

wxiaoguang commented May 6, 2022 •

edited

Loading

Gusted commented May 6, 2022 •

edited

Loading