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 reviewing PR UX #19612

Merged
merged 13 commits into from
May 7, 2022
Merged

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented May 4, 2022

  • 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:
image

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

@Gusted Gusted added this to the 1.17.0 milestone May 4, 2022
@Gusted 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
@codecov-commenter

This comment was marked as off-topic.

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

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

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-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
@wxiaoguang
Copy link
Contributor

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
Copy link
Member

delvh commented May 5, 2022

@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
Copy link
Contributor

wxiaoguang commented May 5, 2022

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
Copy link
Member

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
Copy link
Contributor

wxiaoguang commented May 5, 2022

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
Copy link
Contributor

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/

image

@silverwind
Copy link
Member

silverwind commented May 5, 2022

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 and others added 4 commits May 5, 2022 22:23
Co-authored-by: silverwind <me@silverwind.io>
Co-authored-by: silverwind <me@silverwind.io>
@Gusted
Copy link
Contributor Author

Gusted commented May 5, 2022

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.

@GiteaBot GiteaBot removed the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 5, 2022
@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label May 5, 2022
@wxiaoguang
Copy link
Contributor

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.

image

Comment on lines 774 to 777
ctx.Data["PendingCodeComments"] = numPendingCodeComments
ctx.PageData["prReview"] = map[string]interface{}{
"pendingCodeComments": numPendingCodeComments,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@wxiaoguang
Copy link
Contributor

wxiaoguang commented May 6, 2022

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

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
Copy link
Contributor Author

Gusted commented May 6, 2022

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.

@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 May 6, 2022
@zeripath
Copy link
Contributor

zeripath commented May 6, 2022

make lgtm work

@techknowlogick techknowlogick merged commit 0eac09e into go-gitea:main May 7, 2022
@wxiaoguang
Copy link
Contributor

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.

zjjhot added a commit to zjjhot/gitea that referenced this pull request May 8, 2022
* 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)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui-interaction Change the process how users use Gitea instead of the visual appearance type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pulse the Review button when a comment is added to a review
9 participants