mrbrdo / spree_mobility

BSD 3-Clause "New" or "Revised" License
2 stars 1 forks source link

Fixed translations buttons click bug, issue #2 #3

Closed Kulgar closed 2 years ago

Kulgar commented 2 years ago

Original issue: https://github.com/spree-contrib/spree_globalize/issues/99 Project issue: https://github.com/mrbrdo/spree_mobility/issues/2

Careful, I don't know if my fix will work with Spree 4.3. I don't know when they added the "Spree:load" event.

mrbrdo commented 2 years ago

Thanks, I'll check later. Quite possible won't work with 4.3, I think they switched to Turbo in newer versions. Will see if it's possible to make it work with both versions.

Kulgar commented 2 years ago

@mrbrdo did another update, I switched to jQuery syntax to easily listen to both spree:load and ready events. It seems to work (and it doesn't duplicate JS features) on Spree 4.4. I'll let you test on Spree 4.3

mrbrdo commented 2 years ago

@Kulgar I can confirm it's working as before with 4.3.1. Therefore I did merge it. Thanks!

However I would ask that you check on 4.4 that these functions are triggered once on page load (actual page load - browser refresh), and not twice? Since I am not sure exactly if spree:load is triggered on page load. In this case, although it may not cause problems at the moment, it would be good to find a better solution if we included JS that was not idempotent. In that case I think Spree might expose the version in JS, which could be used to determine the correct approach, or you could expose it if not.

mrbrdo commented 2 years ago

Actually, I forgot that spree_backend includes this (implementation from 4.3):

Spree.ready = function (callback) {
  return jQuery(document).on('page:load turbolinks:load', function () {
    return callback(jQuery)
  })
}

If that works correctly with 4.4 (not called twice on load), it should work with 4.3 too :)

Kulgar commented 2 years ago

Well, yes, I tested and it doesn't get called twice, that's what I meant by "(and it doesn't duplicate JS features)". Haha. So, I should call "Spree.ready(function() { ... })" ?

mrbrdo commented 2 years ago

Yeah just try if that works and if you can make a PR with that change it would be much appreciated. :)