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

[CD-515] GTM deferred #383

Closed
wants to merge 2 commits into from
Closed

[CD-515] GTM deferred #383

wants to merge 2 commits into from

Conversation

rupl
Copy link
Contributor

@rupl rupl commented Jan 23, 2024

CD-515

Forked the gtm_barebones module after confirming that the maintainer did not want to adopt the change.

  • Inline JS converted to an external script with defer attribute.
  • All three config values are output to drupalSettings so that the module can output a static JS file that never needs rebuilding, even if config changes.
  • No changes to any of the config; find-replace in Ansible should be possible to adopt this module (i.e. change gtm_barebones to ocha_gtm)

If people are happy with this, I'll put it in a standalone repo using the Drupal module as an upstream so that we can get security updates.

@rupl rupl marked this pull request as draft January 23, 2024 14:46
Copy link
Contributor

@lazysoundsystem lazysoundsystem left a comment

Choose a reason for hiding this comment

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

Haven't diffed it against the original module, but it all looks right.

@attiks
Copy link
Contributor

attiks commented Jan 23, 2024

My 2 cents

  • isn't it easier to use a patch instead of renaming? or will you clone it into unocha/gtm_barebones
  • why not add a route for ocha_gtm/js/gtm.js that returns the js code, no more need for settings

@lazysoundsystem
Copy link
Contributor

Some things that need fixing picked up in tests:

FILE: /srv/www/html/modules/custom/ocha_gtm/ocha_gtm.module
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | ERROR | [x] Missing file doc comment
----------------------------------------------------------------------


FILE: /srv/www/html/modules/custom/ocha_gtm/js/gtm.js
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 1 | WARNING | File appears to be minified and cannot be processed
----------------------------------------------------------------------


FILE: /srv/www/html/modules/custom/ocha_gtm/src/Hooks/OchaGtmHooks.php
----------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
----------------------------------------------------------------------
  10 | WARNING | [x] Unused use statement
 102 | ERROR   | [x] Missing function doc comment
----------------------------------------------------------------------

@rupl
Copy link
Contributor Author

rupl commented Jan 24, 2024

why not add a route for ocha_gtm/js/gtm.js that returns the js code, no more need for settings

The reason was because it's a days-long battle for me to add a route to Drupal, but adding a static JS file took me a few minutes. I made the new UN-OCHA repo (not renamed) and will ask for your help if I can't figure it out by day's end.

@rupl
Copy link
Contributor Author

rupl commented Feb 26, 2024

So, we did end up forking the module into our GH org. Closing this prototype PR as it served its purpose.

@rupl rupl closed this Feb 26, 2024
@rupl rupl deleted the CD-515-gtm-deferred branch February 26, 2024 13:25
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.

3 participants