-
Notifications
You must be signed in to change notification settings - Fork 699
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
Generated App doesn't work in Safari/Firefox #1506
Conversation
lib/generators/shopify_app/home_controller/home_controller_generator.rb
Outdated
Show resolved
Hide resolved
lib/generators/shopify_app/home_controller/templates/index.html.erb
Outdated
Show resolved
Hide resolved
lib/generators/shopify_app/home_controller/templates/index.html.erb
Outdated
Show resolved
Hide resolved
lib/generators/shopify_app/home_controller/templates/index.html.erb
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tracking this down and getting this fix in!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit
document.addEventListener("DOMContentLoaded", async function() { | ||
displayProducts(); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we simplify this?
document.addEventListener("DOMContentLoaded", async function() { | |
displayProducts(); | |
}); | |
document.addEventListener("DOMContentLoaded", displayProducts); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right! I was trying to do this but I was using displayProducts()
instead of displayProducts
lesson learn lol. Thanks for this!
What this PR does
The
index.html.erb
had a messed up import. The root of the problem seems to still be a mystery, but there is a problem with how shims work in Rails 7. I found that if I remove the event listener and useimport
, instead ofimport()
then everything seems to work.Q: Why do we need to remove the event listener?
The only real answer I have is that the event is already gone after the shim gets loaded. I figured this out with console logs but forgot to screenshot it. If this seems weird and you want me to get another screenshot I can.
closes Embedded apps not working on some browsers
Reviewer's guide to testing
Honestly the best way to test this is create an app in rails 6 and 7. If you have a previous app I think you should be able to use
rails generate shopify_app
on this branch, and yourindex.html.erb
file should change slightly.Things to focus on
import
instead ofimport()
Checklist
Before submitting the PR, please consider if any of the following are needed:
CHANGELOG.md
if the changes would impact usersREADME.md
, if appropriate./docs
, if necessary