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 jQuery event shorthand is deprecated - focus, focusout, hover #37

Merged
merged 3 commits into from Nov 14, 2022

Conversation

leemyongpakvn
Copy link
Contributor

@leemyongpakvn leemyongpakvn commented Jun 13, 2022

Questions Answers
Description? Fix well-known deprecated warnings following jquery-validation/jquery-validation#2243 and https://api.jquery.com/focus, https://api.jquery.com/focusout
Type? bug fix
BC breaks? no
Deprecations? yes
Fixed ticket? Fixes {paste the issue here}.
How to test? 1. In terminal run:
cd themes/classic/_dev
npm run build
2. In FO: Clear Web browser cache
3. In BO Configure/Advanced/Performance: Clear cache then Disable Smart cache for JavaScript in CCC;
4. Add some products to Cart, then open Cart page and look at Web browser console to see the warnings (from theme.js) still there or not
Possible impacts? Prestashop 1.7.8.5, FireFox browser

ps178_jquery_focus_focusout_hover_event_shorthand_deprecated

@NeOMakinG NeOMakinG added the Waiting for QA Status: Waiting for QA feedback label Jun 16, 2022
@hibatallahAouadni hibatallahAouadni self-assigned this Jun 16, 2022
@leemyongpakvn
Copy link
Contributor Author

@hibatallahAouadni hibatallahAouadni removed their assignment Jun 21, 2022
@khouloudbelguith khouloudbelguith self-assigned this Jun 21, 2022
Copy link
Contributor

@khouloudbelguith khouloudbelguith left a comment

Choose a reason for hiding this comment

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

Hi @leemyongpakvn,

I tried this theme using branch 8.0.x.
Using Chrome Version 94.0. and Firefox 101.0.1 (64 bits).

  • In the FO > Click on any product to check the product details page
  • See error in the console

The error .hover is still displayed
image

I attached a screen record

Untitled_.Jun.21.2022.5_09.PM.mp4

Could you please confirm that only the warning related to .focus & .focusout & .hover will be fixed in this PR?

Thank you!

@khouloudbelguith khouloudbelguith added Waiting for author and removed Waiting for QA Status: Waiting for QA feedback labels Jun 21, 2022
@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Jun 22, 2022

@khouloudbelguith I confirm that this PR will fix the warning related to .focus & .focusout & .hover come from classic theme (theme.js) in Cart page of PS 1.7.8.x .
I'm not sure about PS 8.0.x, Product page or warnings come from other modules (as I know till now js code of ps_searchbar and productcomments modules make the similar warning also).
Please be sure that you rebuild theme.js with npm run build in _dev directory after apply changes to 3 component js files following https://devdocs.prestashop.com/1.7/themes/getting-started/asset-management/webpack/#installing-webpack.

@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Jul 27, 2022

@khouloudbelguith Tested OK with PS 8.0.0 also. Please Disable Smart cache for JavaScript in configure/advanced/performance/CCC (Combine, Compress and Cache) and clear web browser cache before test again.

@leemyongpakvn leemyongpakvn changed the title Fix jQuery.fn.focus() event shorthand is deprecated Fix jQuery event shorthand is deprecated - focus, focusout, hover Jul 27, 2022
@kpodemski kpodemski added Waiting for QA Status: Waiting for QA feedback and removed Waiting for author labels Aug 25, 2022
@kpodemski
Copy link
Contributor

@khouloudbelguith remember that it might not fix all the issues in the developer console, but we should have less as some could be from some modules, etc.

@leemyongpakvn
Copy link
Contributor Author

@NeOMakinG Will I need to add assets/js/theme.js, or maintainer will do while releasing new version?

@NeOMakinG
Copy link

@leemyongpakvn if assets are pushed to the repository, I would suggest you to push the theme.js inside your PR directly so we are sure that we don't forget to push them!

@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Nov 7, 2022

@NeOMakinG I don't see assets folder in both this repository Code and the Releases Source code zips. BTW, How to test? is updated with theme.js rebuild and check.

@NeOMakinG
Copy link

NeOMakinG commented Nov 7, 2022

@leemyongpakvn yeah because I'm pretty sure it's added via composer and then build on the PS release, you shouldn't push assets I guess, the assets folder is even inside the gitignore 👍

@hibatallahAouadni hibatallahAouadni self-assigned this Nov 10, 2022
Copy link
Contributor

@hibatallahAouadni hibatallahAouadni left a comment

Choose a reason for hiding this comment

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

Hello @leemyongpakvn

I reproduce the issue even when I disabled the cache, see the attached screen record below:

pr_37.webm

Please check and feedback.

Thanks!

@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Nov 12, 2022

@hibatallahAouadni I have added two more steps in How to test? (step 2 & 3). I can not run your .webm screencast, so please send in another popular format (.mov, .mp4), or just a screenshot of the Web browser console log.

@hibatallahAouadni hibatallahAouadni removed their assignment Nov 14, 2022
@leemyongpakvn
Copy link
Contributor Author

@kpodemski and @NeOMakinG I thought Step 1, 2, 3 in this How to test? is a must-to-know in QA cookbook 🙏 It is boring to repeat these steps in every PR that has CSS, JS changes with Webpack.

@kpodemski
Copy link
Contributor

@leemyongpakvn I'm pretty sure they did that :) I think not all console errors will disappear because, if I recall correctly productcomments module with the fixes hasn't been released yet, which means that there will be errors, but fewer. Am I right?

@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Nov 14, 2022

@kpodemski Your recalling is right. I informed it with khouloudbelguith on Jun 22, and emphasized checking errors come from theme.js in Web console in additional How to test Step 4 on Nov 12. 😋

@MhiriFaten MhiriFaten self-assigned this Nov 14, 2022
Copy link

@MhiriFaten MhiriFaten left a comment

Choose a reason for hiding this comment

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

Hello @leemyongpakvn ,

I tested this theme

I followed the process with

  • rebuild theme.js with npm run build in _dev and
  • Clear cache then Disable Smart cache for JavaScript in CCC

The issue is well fixed ✔️ and there is no longer any warning related to .focus & .focusout & .hover

image

So it's QA ✔️
Thank you

@MhiriFaten MhiriFaten added QA ✔️ Status: QA-Approved and removed Waiting for QA Status: Waiting for QA feedback labels Nov 14, 2022
@MhiriFaten MhiriFaten removed their assignment Nov 14, 2022
@kpodemski kpodemski added this to the 2.0.7 milestone Nov 14, 2022
@kpodemski kpodemski merged commit bbae998 into PrestaShop:develop Nov 14, 2022
@kpodemski
Copy link
Contributor

Thanks, @leemyongpakvn !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
QA ✔️ Status: QA-Approved
Projects
None yet
7 participants