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

Removed unused jQuery 1.10.2, updated jQuery 1.12.1 to 1.12.4 #2442

Merged
merged 2 commits into from
Aug 25, 2022
Merged

Removed unused jQuery 1.10.2, updated jQuery 1.12.1 to 1.12.4 #2442

merged 2 commits into from
Aug 25, 2022

Conversation

fballiano
Copy link
Contributor

If you search for "jquery-1.10.2" you'll see that there are no occurrencies, since version 1.12 is present and used in RWD theme

Schermata 2022-08-17 alle 21 54 18

Maybe we could target this to v20, in case we think some extension uses the old version of the javascript, what do you think?

@github-actions github-actions bot added the JavaScript Relates to js/* label Aug 17, 2022
@addison74
Copy link
Contributor

I just wanted to propose this change. I would go ahead and bring the last version from branch 1, which is 1.12.4. so the JQuery in OpenMage will be up to date.

@fballiano
Copy link
Contributor Author

should we update it in this same PR?

@addison74
Copy link
Contributor

Maybe we could target this to v20, in case we think some extension uses the old version of the javascript, what do you think?

We will faced this question from now on, if any developer has used the files we want to delete or update, but we will never know if we don't step forward. I think it is more important to have all the code up to date than to leave old, outdated. Surely if there will be an issue in the future those who use OM will report it.

should we update it in this same PR?

I have nothing against.

@fballiano
Copy link
Contributor Author

ok i guess we can leave this PR in the v19 branch.

and i guess it also makes sense to update the jquery 1.12 to 1.12.4 in this same PR, will to it asap (working on another issue at the moment)

@addison74
Copy link
Contributor

Please upload both uncompressed and minified versions https://releases.jquery.com/jquery/. If possible the map file.

@justinbeaty
Copy link
Contributor

justinbeaty commented Aug 17, 2022

I say remove both versions, add 1.12.4, and update app/design/frontend/rwd/default/layout/page.xml

Modules often just bundled their own version of jQuery, but in the case it was actually needed, it's an easy fix. Plus many old-school Magento installs probably just extract the releases on top of the old install anyway, and thus wouldn't have it removed.

Edit: we could also place it at js/lib/jquery/jquery-1.12.x.js in case there is ever another release (seems unlikely).

@fballiano
Copy link
Contributor Author

i don't think we can't remove it easily no?

Schermata 2022-08-17 alle 22 42 56

@addison74
Copy link
Contributor

addison74 commented Aug 17, 2022

I don't think a new version will ever be released in branch 1. Leave the modification only in OM-20 and modify the XML file according to the new version.

I looked through my extensions and no developer uses the JQuery library added specifically for the RWD theme, but each put their own newer or older version in the /skin directory.

@justinbeaty
Copy link
Contributor

i don't think we can't remove it easily no?

I meant to add in 1.12.4 as a replacement, so that 1.12.4 is the only version left. Or were you thinking keep both 1.12.1 and 1.12.4?

@fballiano
Copy link
Contributor Author

I don't think a new version will ever be released in branch 1. Leave the modification only in OM-20 and modify the XML file according to the new version.

yes I agree, but I thought we said to keep this PR in the v19 branch too no?

@fballiano
Copy link
Contributor Author

I meant to add in 1.12.4 as a replacement, so that 1.12.4 is the only version left. Or were you thinking keep both 1.12.1 and 1.12.4?

we'll only have the last one ;-)

@addison74
Copy link
Contributor

yes I agree, but I thought we said to keep this PR in the v19 branch too no?

It can be used in OM-19 as well. Basically if someone uses the classic RWD theme in Frontend he will be just fine.

@github-actions github-actions bot added Component: Page Relates to Mage_Page Template : rwd Relates to rwd template XML Layout labels Aug 17, 2022
@fballiano
Copy link
Contributor Author

I think everything should be done now

addison74
addison74 previously approved these changes Aug 17, 2022
justinbeaty
justinbeaty previously approved these changes Aug 17, 2022
@justinbeaty
Copy link
Contributor

I have some potential feedback / ideas to test tomorrow if we can wait to merge this.

@fballiano fballiano changed the title Removed unused jQuery 1.10.2, since version 1.12.1 is already present and used Removed unused jQuery 1.10.2, updated jQuery 1.12.1 to 1.12.4 Aug 18, 2022
@justinbeaty
Copy link
Contributor

I was trying to see if it was feasible to install jQuery via composer. There are a few ways to do it as you can see here: justinbeaty@ab05de9 (inspired by this guide)

But it doesn't seem to work when installing openmage using magento-core-composer-installer. Maybe eventually we can modify magento-hackathon/magento-composer-installer to let us install JS libraries too.

So I'm back to approving this PR as it is. :)

tmewes
tmewes previously approved these changes Aug 18, 2022
@addison74
Copy link
Contributor

Even if I have approved it, I will check if errors appear in the browser console when I visit pages in the Frontend. If there are errors they are not from the new version anyway, but maybe we will fix them in the future. I hope it won't be necessary.

@sreichel sreichel dismissed stale reviews from justinbeaty and addison74 via 75780ae August 18, 2022 22:44
addison74
addison74 previously approved these changes Aug 19, 2022
@fballiano fballiano dismissed stale reviews from addison74 and tmewes via 8e945dd August 20, 2022 15:34
addison74
addison74 previously approved these changes Aug 21, 2022
tmewes
tmewes previously approved these changes Aug 21, 2022
@fballiano fballiano merged commit 8698ea6 into OpenMage:1.9.4.x Aug 25, 2022
@fballiano fballiano deleted the remove_jquery_110 branch August 25, 2022 08:24
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 8698ea6. ± Comparison against base commit 620cc0a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Page Relates to Mage_Page JavaScript Relates to js/* Template : rwd Relates to rwd template XML Layout
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants