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 embedded shops failing javascript because jQuery is try to be accessed before being loaded #5996

Conversation

andresgutgon
Copy link
Contributor

What? Why?

We check for $(document).ready when jQuery is not downloaded yet in the browser. The solution is to check if document is ready with plain DOM javascript event DOMContentLoaded

This is the error in a client's page biodynamic.org.uk/where-to-buy when embedding the map:

image

Closes #2723 (Maybe, I'm not sure)

What should we test?

After deployed the change check browser console on this page biodynamic.org.uk/where-to-buy and also check the issue. No Zoom Buttons on Maps in Embedded Groups

Changelog Category: Fixed

accessed before being loaded
Show we check for $.ready when jQuery is not downloaded yet in the
browser. The solution is to check if document is ready with plain DOM
javascript event `DOMContentLoaded`
Copy link
Contributor

@sauloperez sauloperez left a comment

Choose a reason for hiding this comment

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

Nice one! nothing beats having a seasoned frontend dev around 😂 💪 Thanks again @andresgutgon !

@filipefurtad0 filipefurtad0 self-assigned this Sep 16, 2020
@filipefurtad0 filipefurtad0 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Sep 16, 2020
@filipefurtad0
Copy link
Contributor

Hey @andresgutgon ,

Using Chrome browser, I verified the console error on the page biodynamic.org.uk/where-to-buy and that zoom commands are missing from the embedded map (pic below, left side.) However, after staging, I'm observing the same error, zoom buttons absent as well (pic below, right side). Also, one sees a violation concerning DOMContentLoaded handler, see below.

image

Maybe this PR needs a second look? What do you think @Matt-Yorkley @sauloperez?
Thank you!

@andresgutgon
Copy link
Contributor Author

Hi @filipefurtad0 it's staging public? Can I check it?

@filipefurtad0
Copy link
Contributor

This was incorrect thinking from my side: I staged the PR on a staging server, but this of course will not address the issue on the website - as this is in production. So, coming to think of it @andresgutgon , this is actually a production test, right?

The only valuable test I think of in staging would be to embed a shop from a staging server in a website, and i) verify the bug and then ii) see its good after the PR.

Maybe we should just go ahead and merge, to check it on the next release? Thoughts?

@luisramos0
Copy link
Contributor

Hmm, embed shopfronts is an unsupported feature now.
I think we have changed the JS code and we havent validated it with a embed shopfront. If there are problems with the JS code here this can break some/all embed shops. Because this is an unsupported feature and I also think the code looks good (3 reviews), I am going to merge it. I also think other devs will be ok with this :-)
🤞

This is going to be merged and disappear from the pipe (usually the tag prod-test remains on the open issue, not the PR), so we need to remember to test this in prod after the release on Tuesday.

@luisramos0 luisramos0 merged commit e99fdeb into openfoodfoundation:master Sep 17, 2020
@luisramos0
Copy link
Contributor

I see we have issue #2723 for this. We dont need to remember the prod-test here. I added the label to #2723 and left it in Test Ready 👍

@andresgutgon andresgutgon deleted the fix/iframe-shop-failing-because-jquery-is-not-loaded branch September 17, 2020 10:35
@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Sep 24, 2020

Hi @andresgutgon ,

The code from this PR is in production. Visiting the page biodynamic.org.uk/where-to-buy and opening the Chrome console shows that the Uncaught ReferenceError is gone. Zoom commands remain absent:

image

What do you think?

@andresgutgon
Copy link
Contributor Author

What I think is that life is hard 😂

I may take another look. At least I think we solved an error here right? I guess, not sure let's see

@filipefurtad0
Copy link
Contributor

filipefurtad0 commented Sep 24, 2020

Yeah, the original error seems to be gone 👍
This PR is now merged so I think we would need a separate PR, should you address this again later on. I'll keep issue #2723 open for now.

Thank you @andresgutgon!

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.

No Zoom Buttons on Maps in Embedded Groups
5 participants