Skip to content

Inject script in footer if theme is using theming v2 api #340

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

abdul
Copy link
Contributor

@abdul abdul commented May 31, 2020

✌️

/cc @zendesk/vegemite

Description

As mentioned in #339

The theme preview is injecting script.js in document_head template which was fine for theming_v1 based themes.

The theming_v2 themes will now have script.js at the end of the page. That means, people need to do some work to migrate their javascript to adopt to this new situation (script.js loads at the end), and test.

This must be fixed to make sure we can have similar (if not same) locally. The order of script matters a lot, and if it's not right it leads to issues.

I have made the change to check api_version, and change the template where script is injected.

Tasks

  • Include comments/inline docs where appropriate
  • Write tests
  • Update changelog here

References

Risks

  • [HIGH | medium | low] Does it work on windows? Unable to verify
  • [HIGH | medium | low] Does it work in the different products (Support, Chat)? End User Request form from Support
  • [HIGH | medium | low] Are there any performance implications? No
  • [HIGH | medium | low] Any security risks? No
  • [HIGH | medium | low] What features does this touch? Local Theme Preview

@abdul
Copy link
Contributor Author

abdul commented May 31, 2020

I will add more changes to this PR, it seems we need to refactor the code to allow injection of style and script tags in different templates.

@abdul
Copy link
Contributor Author

abdul commented May 31, 2020

I have updated the code, please look at it. I am going to to refactor it better. I am not a ruby developer so didn't know how to pass options[:port] to Common helper? I don't think, Common Helper should know about Theme, and it's internals.

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.

1 participant