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

Replace usages of jQuery.live usages and add polyfill #15900

Merged
merged 6 commits into from Oct 16, 2019

Conversation

@matthieu-rolland
Copy link
Contributor

matthieu-rolland commented Oct 10, 2019

Questions Answers
Branch? develop
Description? Replace usage of jQuery.live with jQuery.on, and add a jQuery.live polyfill.
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #15882
How to test? Check that order page in BO works fine again.

This change is Reviewable

@matthieu-rolland matthieu-rolland requested a review from PrestaShop/prestashop-core-developers as a code owner Oct 10, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:jquery-live branch from 1ed2e11 to f779462 Oct 14, 2019
@matthieu-rolland matthieu-rolland added WIP and removed WIP labels Oct 14, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:jquery-live branch from 0735406 to 711ef4d Oct 14, 2019
@matthieu-rolland matthieu-rolland force-pushed the matthieu-rolland:jquery-live branch from 711ef4d to b7d426b Oct 14, 2019
@matthieu-rolland matthieu-rolland removed the WIP label Oct 14, 2019
Copy link
Contributor

PierreRambaud left a comment

Reviewed 5 of 9 files at r1, 1 of 3 files at r3.
Reviewable status: 6 of 16 files reviewed, 4 unresolved discussions (waiting on @matthieu-rolland and @Progi1984)


admin-dev/themes/default/template/controllers/addresses/helpers/form/form.tpl, line 52 at r3 (raw file):

		{else}
			<script type="text/javascript">
			$('body').on('blur', 'input[name=email]', function(e)

Always prefer to attach event on document , attaching event to 'body' is the slowest method ^^'


admin-dev/themes/default/template/controllers/carriers/helpers/form/form.tpl, line 31 at r3 (raw file):

	($("input[name='is_free']:checked").val() == 0) ? $('#shipping_costs_div').show('toggle'): $('#shipping_costs_div').hide();

	$('body').on('change', "input[name='is_free']", function() {ldelim}

same here


admin-dev/themes/default/template/controllers/orders/form.tpl, line 128 at r3 (raw file):

			useCart('{$cart->id|intval}');
		{/if}
		$('body').on('click', '.delete_product', function(e) {

same everywhere


admin-dev/themes/default/template/controllers/tags/helpers/form/form.tpl, line 67 at r1 (raw file):

Previously, matthieu-rolland (Matthieu Rolland) wrote…

done !

I don't see any differences.

@matthieu-rolland

This comment has been minimized.

Copy link
Contributor Author

matthieu-rolland commented Oct 15, 2019

@PierreRambaud As requested, I replaced the 'body' string by a document object in jquery selectors regarding the use of jQuery.on(). 👍

Copy link
Contributor

PierreRambaud left a comment

Reviewed 7 of 7 files at r2, 1 of 3 files at r3, 4 of 7 files at r4.
Reviewable status: 13 of 16 files reviewed, 4 unresolved discussions (waiting on @matthieu-rolland, @PierreRambaud, and @Progi1984)

Copy link
Contributor Author

matthieu-rolland left a comment

Reviewable status: 13 of 16 files reviewed, 4 unresolved discussions (waiting on @PierreRambaud and @Progi1984)


admin-dev/themes/default/template/controllers/addresses/helpers/form/form.tpl, line 52 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

Always prefer to attach event on document , attaching event to 'body' is the slowest method ^^'

Done.


admin-dev/themes/default/template/controllers/carriers/helpers/form/form.tpl, line 31 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

same here

Done.


admin-dev/themes/default/template/controllers/orders/form.tpl, line 128 at r3 (raw file):

Previously, PierreRambaud (GoT) wrote…

same everywhere

Done.


classes/controller/AdminController.php, line 2710 at r1 (raw file):

Previously, matthieu-rolland (Matthieu Rolland) wrote…

In the end I implementend your suggestion @Progi1984 :D . This js file is not to be fetched via npm in the legacy backoffice.

Done.

@Robin-Fischer-PS Robin-Fischer-PS self-assigned this Oct 16, 2019
@Robin-Fischer-PS Robin-Fischer-PS added this to the 1.7.7.0 milestone Oct 16, 2019
Copy link
Contributor Author

matthieu-rolland left a comment

Reviewable status: 13 of 16 files reviewed, 4 unresolved discussions (waiting on @PierreRambaud, @Progi1984, and @Robin-Fischer-PS)


admin-dev/themes/default/template/controllers/orders/form.tpl, line 128 at r3 (raw file):

Done. PierreRambaud (GoT) is: No elements found. Consider changing the search query. Blocking:

Quoted 77 lines of code…
    opposed to resolving the discussion while waiting on somebody else.
    Dismiss from
  this discussion
  •
  unresolved discussions.
        
        This is advanced stuff, you don't need it to get started.You are:  Discussing:
        contributing to the discussion but neutral on resolving.
         Satisfied: in favor of resolving and ending the discussion.
         Blocking:
        opposed to resolving the discussion while waiting on somebody else.
       Working:
        holding the discussion unresolved while working on something related.
        No elements found. Consider changing the search query. List is empty. 
                
              




  $(document).on('click', '.delete_product', function(e) {








  e.preventDefault();







  e.preventDefault();








  var to_delete = $(this).attr('rel').split('_');







  var to_delete = $(this).attr('rel').split('_');








  deleteProduct(to_delete[1], to_delete[2], to_delete[3]);







  deleteProduct(to_delete[1], to_delete[2], to_delete[3]);
> EXPANDEXPAND
Quoted 36 lines of code…
            Collapsed:
          1461 lines
        with
        
          no changes
        


        
      
    
      
      
        
  single

  auto
130…admin-dev/themes/default/template/controllers/specific_price_rule/…helpers/form/form.tpl⊥r2r3r4




   







   
> EXPANDEXPAND
Quoted 39 lines of code…
            Collapsed:
          318 lines
        with
        
          no changes
        


        
      
    
      
        
          
            Files hidden because they have no changes or open comments:
          
          
            (show all)
          
        
        admin-dev/themes/default/template/controllers/tags/helpers/form/form.tpl
        
      
      
    
      
        
        admin-dev/themes/default/template/helpers/form/assoshop.tpl
        
      
      
    
      
      
        
  single

  auto
130…</details>
@matks
matks approved these changes Oct 16, 2019
Copy link
Contributor

matks left a comment

Reviewed 7 of 7 files at r2, 2 of 3 files at r3, 7 of 7 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @PierreRambaud, @Progi1984, and @Robin-Fischer-PS)

@matks matks dismissed Progi1984’s stale review Oct 16, 2019

Requested changes applied

@matthieu-rolland matthieu-rolland merged commit 03ce1d9 into PrestaShop:develop Oct 16, 2019
2 of 3 checks passed
2 of 3 checks passed
code-review/reviewable 4 discussions left (PierreRambaud, Progi1984, Robin-Fischer-PS)
Details
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.