Skip to content

Allow different vendor - #170

Open
MegEdnazednav wants to merge 2 commits into
t-jindai:masterfrom
MegEdnazednav:master
Open

Allow different vendor#170
MegEdnazednav wants to merge 2 commits into
t-jindai:masterfrom
MegEdnazednav:master

Conversation

@MegEdnazednav

Copy link
Copy Markdown

Hello,
This change goes together with the PR made in https://github.com/tomoyukikashiro/gatsby-plugin-html2amp/pull/73

@t-jindai t-jindai left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Thank you for your contribution !
Could you check my feedback ?

Comment thread README.md

name | default value | note
------------ | ------------- | -------------
------------ | ------------- | -------------

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can you add description about the new option here?

const config = utils.getRelativeFile(options.gaConfigPath, options.cwd)
const $script = $('<script async custom-element="amp-analytics" src="https://cdn.ampproject.org/v0/amp-analytics-0.1.js" />')
const $config = $(`<amp-analytics type="googleanalytics"/><script type="application/json">${config}</script></amp-analytics>`)
const $config = $(`<amp-analytics type=${options.vendor}><script type="application/json">${config}</script></amp-analytics>`)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

IMO, vender option might be ambiguous. 🤔 User may think what kind of vender it is.

How about analyticsVendor ?

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.

2 participants