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

Fix JavaScript errors and adjust styling #7

Closed
wants to merge 2 commits into from
Closed

Fix JavaScript errors and adjust styling #7

wants to merge 2 commits into from

Conversation

barrymieny
Copy link

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

Require jQuery and define the `unsafeWindow` object with at least a `menuOpened` attribute
@Smeulf
Copy link
Owner

Smeulf commented Jul 29, 2022

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.

@Smeulf Smeulf closed this Jul 29, 2022
@barrymieny
Copy link
Author

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

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

Smeulf commented Jul 30, 2022

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

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

Smeulf commented Jul 30, 2022

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

Smeulf commented Aug 3, 2022

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

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

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.

None yet

3 participants