Skip to content

Add namespace to global click event #16833

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 1 commit into from
Closed

Conversation

derekhu
Copy link

@derekhu derekhu commented Aug 27, 2021

Sovle : #15687

@silverwind
Copy link
Member

silverwind commented Aug 27, 2021

I don't understand the purpose of this change. What is that cleanhash class? I don't think we use it, so this change would effectively kill that click handler as it is.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Aug 27, 2021
@derekhu
Copy link
Author

derekhu commented Aug 27, 2021

I don't understand the purpose of this change. What is that cleanhash class? I don't think we use it, so this change would effectively kill that click handler as it is.

Hi, cleanhash in $(document).on("click.cleanhash") is jQuery event namespace, it is not class etc.

As I mentioned in #15687, I don't want gitea to clean hash automaticly when I click the blank area of issue (after click some certain comment with note id in hash) . I want to keep the comment-link with note id in url#hash after click blank area of the issue. In my work, I offen open many tabs of issue, and with focused comment highlight and url#hash, I can quickly go back to work from issue to issue. And this is what the Gitlab default does.

Gitea, by default , will clean the hash when I click blank area just like Github does.

Also, Gitea provided super great custom way, to add my own js , including overwrite the whole index.js. So there is a way for me to turn off the default behavior.

If the global click event has jQuery's event namespace, then I don't have to overwrite the whole index.js, I only need to add my own js in footer.tmpl, and add simple line of code: $(document).off("click.cleanhash") to turn off the default behavior in my custom code. And Gitea keeps the same default behavior unchanged for other people.

That is the reason why I want to add event.namespace to global click event handle. without any other negative effects .

Thank you for the Great Gitea. It helps me a lot.

FYI: jQuery event.namespace
https://api.jquery.com/event.namespace/

@silverwind
Copy link
Member

silverwind commented Aug 28, 2021

I see, wasn't aware of that jQuery feature. Do you think it can be done without jQuery too (feel free to replace .on with .addEventListener)? I don't like to deepen the integration with it any more and I think we should strive to eliminate the jQuery dependency eventually because the DOM APIs are sufficient and much faster.

@derekhu
Copy link
Author

derekhu commented Aug 28, 2021

I see, wasn't aware of that jQuery feature. Do you think it can be done without jQuery too (feel free to replace .on with .addEventListener)? I don't like to deepen the integration with it any more and I think we should strive to eliminate the jQuery dependency eventually because the DOM APIs are sufficient and much faster.

yeah, I agree. remove jQuery just like github did. I am not a jQuery fan.

So when?

It seems that I have to continue to overwrite the whole index.js to turn off the feature of auto clean hash. Maybe for another months.
And keep overwriting each time after updating Gitea version.

it’s okay, I already did that. While the Gitea already provided great custom way.

thank you for the Great Gitea, again .

@silverwind
Copy link
Member

silverwind commented Aug 28, 2021

I think I get it now what you want. You want to unregister our event handler in a userscript to implement custom behavior with hashes. While I think the best solution is probably to add a user-facing option, we simply don't have the infrastructure to support such options easily. What I could see being done is to use .addEventListener and expose the listener function on window, e.g.

window._listeners = {};
window._listeners.hashClickListener = () => {/* event handler here */};
document.addEventListener('click', window._listeners.hashClickListener);

That way, you could easily unregister it from userscripts.

@derekhu
Copy link
Author

derekhu commented Aug 28, 2021

I think I get it now what you want. You want to unregister our event handler in a userscript to implement custom behavior with hashes. While I think the best solution is probably to add a user-facing option, we simply don't have the infrastructure to support such options easily. What I could see being done is to use .addEventListener and expose the listener function on windows, e.g.

window._listeners.hashClickListener = () => {}
document.addEventListener('click', window._listeners.hashClickListener);

That way, you could easily unregister it by name from userscripts.

Yeah. It’s a way. However global variables especially the handlers in window is what frontend programmers should avoid to. Gitea need to design the custom way of that properly. But it’s a way, if Gitea team can accept it, it is okay for me:-)

@silverwind
Copy link
Member

Yeah, I don't like polluting window either. We already do it here for the code editor. Maybe there is a better way.

@wxiaoguang wxiaoguang mentioned this pull request Sep 25, 2021
@6543
Copy link
Member

6543 commented Oct 16, 2021

Can you try to merge main into feature branch?

@wxiaoguang
Copy link
Contributor

I think the behavior can be fixed by a capture event listener.

https://stackoverflow.com/questions/12462721/does-stoppropgation-stop-the-event-from-propagating-in-the-capture-phase

Then we do not need to add event namespaces one by one.

I will close this issue and PR, feel free to feedback.

@wxiaoguang wxiaoguang closed this Nov 19, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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.

5 participants