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

clean: use native JS methods instead of JQuery #1845

Merged

Conversation

shenlebantongying
Copy link
Collaborator

I know a tiny bit of jQuery years ago, and I am not motivated to learn it more. Because learning standardized JS can be useful elsewhere but learning jQuery is more or less useless to me.

The $_$ 🤑 is weird. Just use jQuery, the real symbol.

The prettier doesn't work for me locally, I adjusted it a little.

Comment on lines 52 to 53
// TODO: document why Jquery cannot be removed or can we?
result += R"(<script src="qrc:///scripts/jquery-3.6.0.slim.min.js"></script>)";
result += R"(<script> var $_$=jQuery.noConflict(); </script>)";
// TODO: document why noCliflict need to be called.
result += R"(<script> jQuery.noConflict(); </script>)";
Copy link
Collaborator Author

@shenlebantongying shenlebantongying Oct 20, 2024

Choose a reason for hiding this comment

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

I have vague memory that says including jQuery actually fixes some dictionaries that forget to include jQuery.

However, I cannot find the source of this claim and the jQuery.noConflict() seems to nullify this claim.

edit: the claim -> https://forum.freemdict.com/t/topic/11094/14

If we cannot remove jQuery, then we should document this fact here @xiaoyifang .

Copy link
Owner

@xiaoyifang xiaoyifang Oct 21, 2024

Choose a reason for hiding this comment

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

I remembered that some dictionaries will include their own version of JQuery which will conflict with the built-in JQuery.

As far as I know, dictionary makers should not dependent on the gd-ng's built-in jquery , This will limit the dictionary's usage and increase difficulty to be used in other Dictionary Software .

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Actually, this include is at the header and noConflict effectively disabled its $ and dictionaries have to include their own one.

I will leave it be for now, there is another place that needs the jQuery and I will do it later.

Comment on lines 68 to 79
<script>
function gd_init_QtWebChannel(){
new QWebChannel(qt.webChannelTransport, function(channel) {
window.articleview = channel.objects.articleview;});
console.log("Qt webchannel ready...");
};

if (document.readyState !== "loading") {
gd_init_QtWebChannel();
} else {
document.addEventListener("DOMContentLoaded", gd_init_QtWebChannel);
};
</script>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think we should move these from .cc to .js. I will probably do it in another PR.

@shenlebantongying shenlebantongying force-pushed the clean/js-jquery branch 4 times, most recently from bb80452 to e9f7b78 Compare October 20, 2024 20:41
@xiaoyifang
Copy link
Owner

The $_$ 🤑 is weird

I agree ,it is just a trick to make sure that no others will use the same variable name.

@shenlebantongying shenlebantongying force-pushed the clean/js-jquery branch 2 times, most recently from a596eed to 98f5bd6 Compare October 21, 2024 04:42
@shenlebantongying
Copy link
Collaborator Author

There is another jQuery, I will remove it another day.

@shenlebantongying shenlebantongying enabled auto-merge (squash) October 21, 2024 04:48
@shenlebantongying shenlebantongying merged commit e6edd34 into xiaoyifang:staged Oct 21, 2024
8 checks passed
Copy link

@shenlebantongying shenlebantongying deleted the clean/js-jquery branch October 21, 2024 05:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants