search

Smeulf / userscripts

3 stars 1 forks source link

Fix JavaScript errors and adjust styling #7

Closed barrymieny closed 2 years ago

barrymieny commented 2 years ago

Fixing the JavaScript issues allows this to work in Safari with https://github.com/quoid/userscripts

Smeulf commented 2 years ago

Hello @barrymieny

Thanks for your contribution, but I will not commit it for different reasons. Let me explain.

First of all, it's not a bug in the code, but a non implement function in the userscript manager you use.

Second, I'm not ready to make use of jQuery only to add a global variable in the script.

And 3rd, because doing it that way would obfuscate the unsafewindow variable provided by Tampermonkey, and I don't want to run into some non regression test at the moment.

Should I need to add jQuery for any other use, I may reconsider my position, but it's currently not on my plans.

Regarding the design, I'll have a look and maybe get more inspired for the next version thanks to you.

Thank for you understanding.

Cheers. Smeulf.

barrymieny commented 2 years ago

My mistake, I assumed that the comment IMPORTANT: This function requires your script to have loaded jQuery in the original waitForKeyElements.js script meant that jQuery was missing. I will use this fix locally only then since it fixed the issue for me.

jesus2099 commented 2 years ago

Yes @Smeulf, I didn't have time to test this PR, but I think you are already using jQuery in the script. 😁

I found some $ functions.

Smeulf commented 2 years ago

Hi guys.

The script doesn't explicitly loads jQuery right? So that means that it's loaded somewhere else in the website. I was not able yet to find where, but it doesn't really matters.

That's also means we must reference jQuery as a dependency, but make sure it's used in the userscript in a 'no conflict' mode. And of course, do not obfuscate the unsafeWindow variable from Tampermonkey (means changing the name of the global variable).

@barrymieny do you want to try to propose a new change based on those requirements? Without the changes for the look and feel, just to fix that particular bug (I prefer one PR per bug or improvement, I find it easier to manage).

Then I'll port the change to extradev if it works fine.

As it's a discussion, as always if I'm wrong somewhere, feel free to correct me :)

Cheers. Smeulf.

barrymieny commented 2 years ago

I know just enough to be dangerous, so I'll stick to using my fix locally.

If I had more time on my hands to look into this I would, but my priorities lie elsewhere.

Smeulf commented 2 years ago

As you wish, I keep this on my to-do list for the future version. Just be warned your version may encounter unexpected behavior because of the imported jQuery without the 'no conflict' option ;)

Smeulf commented 2 years ago

Hi @barrymieny, hi @jesus2099,

I just created a new branch DEV_jQuery where I pushed a change I think match the requirements.

If you want to try and post some feedback before I push it on master so everyone gets it. That would be very nice of you.

Thanks. Smeulf.

jesus2099 commented 2 years ago

If I were you I would just have got the script free of jQuery. 😜