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

Move a script loading from a snippet to a template to avoid an issue. #3263

Closed

Conversation

eugenekasimov
Copy link
Contributor

@eugenekasimov eugenekasimov commented Feb 8, 2024

PR Summary:

This PR fixes an issue that occurred in the console In the original PR which was supposed to be a start point for all further PRs.

The idea is to get approvals for this PR and use it as a start point for the further PRs instead of this one.

@sofiamatulis asked me to keep her PR as it is so we are not aiming to merge this PR now but we will merge the future PRs into this one 🙂.

Why are these changes introduced?

A loading script <script src="{{ 'quick-add-bulk.js' | asset_url }}" defer="defer"></script> was sitting in the snippet and that way the same variable was declared as many times as the snippet was rendered on the same page (template). This caused multiple issues in the console 👇

Screenshot 2024-02-07 at 4 10 07 PM

What approach did you take?

I move the loading script from the snippet to the template and add a condition when we want to load the script.

Visual impact on existing themes

There is no visual impact.

Testing steps/scenarios

  • Step 1

Demo links

Checklist

Sorry, something went wrong.

Copy link
Member

@dan-menard dan-menard left a comment

Choose a reason for hiding this comment

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

Makes sense to me 👍

Copy link
Contributor

@andrewetchen andrewetchen left a comment

Choose a reason for hiding this comment

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

This looks good to me!

@sofiamatulis asked me to keep her PR as it is so we are not aiming to merge this PR now but we will merge the future PRs into this one 🙂.

I'm aligned with the above!

@sofiamatulis
Copy link
Contributor

@eugenekasimov this PR is not relevant anymore. Can we close it? I will close the old prototypes too :)

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.

None yet

5 participants