Improve reviewing PR UX

[Gusted]

• 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]

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.

[wxiaoguang]

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]

@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]

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]

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]

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]

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]

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.

[Gusted]

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]

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]

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

[wxiaoguang]

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]

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]

make lgtm work

[wxiaoguang]

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.