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

Fixed translations buttons click bug, issue #2 #3

Merged
merged 2 commits into from
May 12, 2022
Merged

Conversation

Kulgar
Copy link
Contributor

@Kulgar Kulgar commented May 12, 2022

Original issue: spree-contrib/spree_globalize#99
Project issue: #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
Copy link
Owner

mrbrdo commented May 12, 2022

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
Copy link
Contributor Author

Kulgar commented May 12, 2022

@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 mrbrdo merged commit e96b0ac into mrbrdo:main May 12, 2022
@mrbrdo
Copy link
Owner

mrbrdo commented May 12, 2022

@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
Copy link
Owner

mrbrdo commented May 12, 2022

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
Copy link
Contributor Author

Kulgar commented May 12, 2022

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
Copy link
Owner

mrbrdo commented May 13, 2022

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

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.

2 participants