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

Converts jQuery statements to native JS and regularize quotation marks #927

Merged
merged 8 commits into from
Nov 27, 2022

Conversation

dheerajdlalwani
Copy link
Contributor

Should potentially fix #920

I have converted as many jQuery functions as I could to native JS.

www/js/app.js Show resolved Hide resolved
Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Thank you very much @dheerajdlalwani. This is looking very good. Just some small comments/queries, and some suggestions to make parts more concise. I haven't tested yet.

www/js/app.js Show resolved Hide resolved
www/js/app.js Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Show resolved Hide resolved
Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Thank you, nearly there pending some minor changes suggested below. I'll do some manual testing.

www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/app.js Outdated Show resolved Hide resolved
Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

All good, many thanks! I'll let you know when I've been able to complete tests. Were you able to test in IE11, for example scrolling to top with and without a ZIM archive loaded?

@dheerajdlalwani
Copy link
Contributor Author

All good, many thanks! I'll let you know when I've been able to complete tests. Were you able to test in IE11, for example scrolling to top with and without a ZIM archive loaded?

I was able to test this in IE 11 with the Zim archive loaded.
I certainly did not test it without loading the Zim archive.
I shall get on to it and perhaps do some more testing.
Thank you!

@Jaifroid
Copy link
Member

OK, thanks! I meant just to test if the scroll to top button (on bottom bar) doesn't produce an exception in console if there is no archive loaded.

@dheerajdlalwani
Copy link
Contributor Author

OK, thanks! I meant just to test if the scroll to top button (on bottom bar) doesn't produce an exception in console if there is no archive loaded.

Yup, I understood what you meant. And no, it does not give an exception. I checked and tested.

@Jaifroid
Copy link
Member

In IE11, articles appear to load, but then don't display correctly. It works fine in Edge Chromium. Can you reproduce?

@Jaifroid
Copy link
Member

Same problem when running the app in IE Mode in Edge Chromium.

@Jaifroid
Copy link
Member

It's something to do with the slide transitions. If you turn the transitions off in Configuration, you can see that the page loads in IE11 and then slides away. A transition is being applied incorrectly it seems. Can you debug?

Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Please debug display of articles in IE11, looking carefully at the slide transition code in uiutil.js. Test it with the transitions turned on and also with them turned off. 😎

@dheerajdlalwani
Copy link
Contributor Author

Oooh yes, I shall get on to it. This seems interesting!

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

Great that you found a solution! Just one last minor suggestion to make the code more concise.

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
www/js/lib/uiUtil.js Show resolved Hide resolved
@Jaifroid
Copy link
Member

I confirm it now works in IE11. 🙂

@Jaifroid
Copy link
Member

There seem to be some problems in the Firefox OS simulator. I'll try to debug these. It's not really evident what this is, but the app appears unresponsive even though no exceptions are being thrown.

@Jaifroid
Copy link
Member

@dheerajdlalwani It turns out that Firefox OS does not recognize the word let, which is the reason we have always used var in this project (oddly, I wasn't aware of this before!).

Although Firefox OS is now deprecated, we still try to keep support "if it's not too complicated". I've tested replacing let with var wherever you used let , with a search-and-replace in my IDE, and it allows the Firefox OS app to work again.

Would you prefer to make this change this yourself, or would you like me to make a commit to your PR? It's completely up to you.

NB you still have to check one-by-one that you aren't changing let in comments, etc., if you use global search-and-replace. And also, please don't change any let that appear in files that are not part of this PR (e.g. in service-worker.js), as these files are not read by Firefox OS and we use modern code there.

In the future, we hope to implement #554 and Babel, and then it will be possible to write modern code and compile for earlier browsers.

@dheerajdlalwani
Copy link
Contributor Author

@dheerajdlalwani It turns out that Firefox OS does not recognize the word let, which is the reason we have always used var in this project (oddly, I wasn't aware of this before!).

Although Firefox OS is now deprecated, we still try to keep support "if it's not too complicated". I've tested replacing let with var wherever you used let , with a search-and-replace in my IDE, and it allows the Firefox OS app to work again.

Would you prefer to make this change this yourself, or would you like me to make a commit to your PR? It's completely up to you.

NB you still have to check one-by-one that you aren't changing let in comments, etc., if you use global search-and-replace. And also, please don't change any let that appear in files that are not part of this PR (e.g. in service-worker.js), as these files are not read by Firefox OS and we use modern code there.

In the future, we hope to implement #554 and Babel, and then it will be possible to write modern code and compile for earlier browsers.

Oh, I didn't know this. I shall fix this right away.

www/js/lib/uiUtil.js Outdated Show resolved Hide resolved
Copy link
Member

@Jaifroid Jaifroid left a comment

Choose a reason for hiding this comment

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

This is now working in the Firefox OS simulator. Many thanks.

@Jaifroid
Copy link
Member

@dheerajdlalwani Let me know when you're happy for this to be squashed/merged.

@dheerajdlalwani
Copy link
Contributor Author

This looks good to me! Go ahead and squash and merge it.

@Jaifroid Jaifroid merged commit 23d596b into kiwix:master Nov 27, 2022
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.

Convert jQuery statements that manipulate CSS to their native equivalents
2 participants