Conversation
tg666
added
documentation
|
Caution Review failedThe pull request is closed. WalkthroughThe changes introduce a new field titled "Package source" in the GTM template, allowing users to specify the location of package files. Additionally, a new section titled "Using other CDN or self-hosted" has been added to the README, providing guidance on configuring the package source and self-hosting instructions. These modifications enhance the documentation and functionality related to package management within the project, including default CDN options and permissions in the GTM template. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Input
participant T as GTM Template
participant JS as JavaScript Logic
U->>T: Set package_source (optional)
T->>JS: Check package_source value
alt package_source provided
JS->>JS: Use provided package_source as scriptBaseUrl
else Not provided
JS->>JS: Construct default URL (using unpkg.com/{version}/)
end
JS->>T: Return scriptBaseUrl for script injection
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- added the field `Package destination` in the GTM template - updated configuration table in the README and added the section "About the package destination" - updated CHANGELOG
tg666
force-pushed
the
feature/self-hosted
branch
from
b6090ac to
a666dd2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
README.md (4)
17-19: Consistent List Item Style in Table of ContentsThe new TOC entry for "About the package destination" has been added. Please verify that the use of asterisks for the unordered list is consistent with the project’s markdown style. If the project convention prefers dashes (–), consider updating these list markers.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
17-17: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
18-18: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
19-19: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
207-230: Comprehensive "About the package destination" SectionThis new section effectively explains the purpose and usage of the
Package destinationfield, including the default behavior (usingunpkg.com) and an alternative usingjsDelivr.com. The step-by-step instructions for self-hosting are clear and informative.A few small improvements are suggested:
- Fenced Code Block Language: The code snippet for the jsDelivr URL (lines 212–214) would benefit from specifying a language for proper syntax highlighting—e.g., use “
text” instead of only “”.- Bare URL Formatting: On line 225, consider wrapping the bare URL (
https://www.example.com/static/cookie-consent/) in angle brackets or markdown link syntax for improved readability.- Phrasing Enhancement: In the self-hosting instructions (e.g. line 230), the phrase “as an overwrite will occur” could be rephrased (e.g., “since the permissions will be reset”) to improve clarity and variety in phrasing.
Overall, these improvements will enhance both the visual rendering and the clarity of the instructions.
🧰 Tools
🪛 LanguageTool
[style] ~219-~219: Consider a more expressive alternative.
Context: ...le to host package files at any URL. To do this, the following steps are required:...(DO_ACHIEVE)
[style] ~228-~228: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...d in the field. Next, the GTM template needs to be modified. In the "Permissions" tab, ...(REP_NEED_TO_VB)
[style] ~230-~230: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...TM template is updated, the permissions need to be modified again, as an overwrite will...(REP_NEED_TO_VB)
[grammar] ~230-~230: The verb ‘overwrite’ does not usually follow articles like ‘an’. Check that ‘overwrite’ is spelled correctly; using ‘overwrite’ as a noun may be non-standard.
Context: ...rmissions need to be modified again, as an overwrite will occur. <img src="docs/images/inje...(A_INFINITIVE)
🪛 markdownlint-cli2 (0.17.2)
212-212: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
225-225: Bare URL used
null(MD034, no-bare-urls)
212-214: Specify Language for Fenced Code BlockThe fenced code block presenting the jsDelivr URL does not have a language specified. To improve readability with syntax highlighting, consider adding a language tag (for example, change from “
” to “text”).🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
212-212: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
225-225: Format Bare URL for ClarityA bare URL is used on line 225. For better markdown rendering and consistency, wrap the URL in angle brackets or use link syntax.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
225-225: Bare URL used
null(MD034, no-bare-urls)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/images/inject-scripts-permissions.pngis excluded by!**/*.png
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)README.md(4 hunks)gtm_template.tpl(3 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~9-~9: Possible typo: you repeated a word.
Context: ...spec/v2.0.0.html). ## [Unreleased] ### Added - Added new field "Package destination" in the ...
(ENGLISH_WORD_REPEAT_RULE)
README.md
[style] ~219-~219: Consider a more expressive alternative.
Context: ...le to host package files at any URL. To do this, the following steps are required:...
(DO_ACHIEVE)
[style] ~228-~228: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...d in the field. Next, the GTM template needs to be modified. In the "Permissions" tab, ...
(REP_NEED_TO_VB)
[style] ~230-~230: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...TM template is updated, the permissions need to be modified again, as an overwrite will...
(REP_NEED_TO_VB)
[grammar] ~230-~230: The verb ‘overwrite’ does not usually follow articles like ‘an’. Check that ‘overwrite’ is spelled correctly; using ‘overwrite’ as a noun may be non-standard.
Context: ...rmissions need to be modified again, as an overwrite will occur. <img src="docs/images/inje...
(A_INFINITIVE)
🪛 markdownlint-cli2 (0.17.2)
README.md
19-19: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
212-212: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
225-225: Bare URL used
null
(MD034, no-bare-urls)
🔇 Additional comments (4)
CHANGELOG.md (1)
9-11: Document New "Package destination" FeatureThe changelog clearly lists the two key additions: the new "Package destination" field in the GTM template and the corresponding README section. This concise description helps users quickly understand the new functionality for self-hosted package files.
🧰 Tools
🪛 LanguageTool
[duplication] ~9-~9: Possible typo: you repeated a word.
Context: ...spec/v2.0.0.html). ## [Unreleased] ### Added - Added new field "Package destination" in the ...(ENGLISH_WORD_REPEAT_RULE)
gtm_template.tpl (3)
48-54: Well-designed new parameter implementationThe addition of the
package_destinationparameter is well-implemented with a clear description that explains the purpose and usage. This enhancement allows users to specify where package files are hosted, which is valuable for teams who want to self-host or use alternative CDNs instead of relying on unpkg.com.
417-423: Good implementation of script URL construction logicThe revised script URL construction logic properly handles the new parameter while maintaining backward compatibility. The code correctly:
- Uses the custom destination if provided, falling back to unpkg.com if not
- Replaces the
{version}placeholder with the actual version value- Ensures the URL ends with a trailing slash
This implementation is robust and handles edge cases appropriately.
2779-2782: Appropriate permissions update for jsDelivrAdding the jsDelivr CDN to the web permissions is necessary and appropriate for the enhancement. This allows users to leverage the jsDelivr CDN as an alternative to unpkg.com.
|
Solves #100 |
|
@CodeRabbit ATM, the readme contains "Package destination" info which aims to the CDN and self-hosted. Change it to "Package host" which is more clean. |
|
Tip For best results, initiate chat on the files or code changes. Now I have a better understanding of the content. Let me create a revised version of the README section as requested, changing "Package destination" to "Package host" and restructuring the content into separate sections for CDN options and self-hosting. @jelen07 Here's the revised section for the README, with "Package destination" renamed to "Package host" and the content split into subsections for CDN options and self-hosting: ## About the package host
The `Package host` field specifies the URL from which package files (JavaScript, CSS, and translation files) will be loaded.
### CDN Options
If the field is left blank, the default CDN used is `unpkg.com`. Alternatively, you can use `jsDelivr.com` by setting the field to:
https://cdn.jsdelivr.net/npm/@68publishers/cookie-consent@{version}/ https://example.com/assets/cookie-consent/ Would you like me to also suggest changes for the GTM template to rename the field there as well for consistency? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
CHANGELOG.md (2)
9-11: Document New Changelog Entries ClearlyThe entries in the "Unreleased" section now capture the new features accurately:
- The addition of the "Package source" field in the GTM template for specifying the location of package files.
- The new "Using other CDN or self-hosted" section in the README to better guide users on configuring how they host package files.
Please ensure these entries remain consistent with the project’s changelog style and that the updated terminology (e.g., "Package source" vs. "Package host") is uniformly applied across all documentation.
🧰 Tools
🪛 LanguageTool
[duplication] ~9-~9: Possible typo: you repeated a word.
Context: ...spec/v2.0.0.html). ## [Unreleased] ### Added - Added new field "Package source" in the GTM t...(ENGLISH_WORD_REPEAT_RULE)
9-11: Review of Changelog FormattingA minor note: static analysis flagged a potential duplication (e.g., the repeated use of "Added" in both the header and bullet points). This repetition is common in changelog formatting per Keep a Changelog standards, but you might consider rewording for conciseness if desired.
🧰 Tools
🪛 LanguageTool
[duplication] ~9-~9: Possible typo: you repeated a word.
Context: ...spec/v2.0.0.html). ## [Unreleased] ### Added - Added new field "Package source" in the GTM t...(ENGLISH_WORD_REPEAT_RULE)
README.md (5)
57-57: Incomplete Markdown Link in Table Row
In the "Package source" field row, the markdown link is missing its closing parenthesis. Consider updating the line to:-| Package source | The URL where the package files are hosted. If the field is not filled, the **unpkg.com** CDN is used. To use a different source, please read the [Using other CDN or self-hosted](#using-other-cdn-or-self-hosted +| Package source | The URL where the package files are hosted. If the field is not filled, the **unpkg.com** CDN is used. To use a different source, please read the [Using other CDN or self-hosted](#using-other-cdn-or-self-hosted) |This will ensure proper markdown rendering and a valid link fragment.
211-216: CDN Alternative Clarification and Code Block Language
The instructions for using an alternative CDN (jsDelivr) are clear and helpful. For better markdown compliance, consider specifying a language for the fenced code block (for example, by addingshafter the opening triple backticks) so that it reads like:```sh https://cdn.jsdelivr.net/npm/@68publishers/cookie-consent@{version}/dist/This small refinement improves readability in supported editors. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 212-212: Fenced code blocks should have a language specified null (MD040, fenced-code-language) </details> </details> --- `217-227`: **Self-Hosted Package Instructions: Grammar and Clarity Improvements** The "Self-hosted package" section is informative; however, consider these suggested refinements for clarity and grammar: - **Rephrase the introductory line:** Instead of “It is possible to host package files at any URL. To do this, the following steps are required:” you might say, “You can host package files on any URL. Follow these steps:” - **Correct the grammatical error:** Change “Also the placeholder `{version}` can used in the field.” to “Also, the placeholder `{version}` can be used in the field.” These improvements will enhance the document's readability. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~219-~219: Consider a more expressive alternative. Context: ...le to host package files at any URL. To do this, the following steps are required:... (DO_ACHIEVE) --- [uncategorized] ~225-~225: A comma may be missing after the conjunctive/linking adverb ‘Also’. Context: ...ww.example.com/static/cookie-consent/`. Also the placeholder `{version}` can used in... (SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA) --- [grammar] ~226-~226: Did you mean “be used”? Context: ...`. Also the placeholder `{version}` can used in the field. Next, the GTM template n... (WILL_BASED_ON) </details> <details> <summary>🪛 markdownlint-cli2 (0.17.2)</summary> 225-225: Bare URL used null (MD034, no-bare-urls) </details> </details> --- `228-230`: **GTM Template Modification Instructions: Reword for Clarity** The text outlining the GTM template changes can be more direct. For example, consider rephrasing as follows: > “Next, update the GTM template: in the ‘Permissions’ tab, add the URL `https://www.example.com/` under ‘Inject scripts’. Note: Each time the GTM template is updated, these permissions must be reconfigured as they may be overwritten.” This version makes the instructions clearer and emphasizes the need for reconfiguration after updates. <details> <summary>🧰 Tools</summary> <details> <summary>🪛 LanguageTool</summary> [style] ~228-~228: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. Context: ...d in the field. Next, the GTM template needs to be modified. In the "Permissions" tab, ... (REP_NEED_TO_VB) --- [style] ~230-~230: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing. Context: ...TM template is updated, the permissions need to be modified again, as an overwrite will... (REP_NEED_TO_VB) --- [grammar] ~230-~230: The verb ‘overwrite’ does not usually follow articles like ‘an’. Check that ‘overwrite’ is spelled correctly; using ‘overwrite’ as a noun may be non-standard. Context: ...rmissions need to be modified again, as an overwrite will occur. <img src="docs/images/inje... (A_INFINITIVE) </details> </details> --- `439-439`: **Update Link Fragment Reference** The link in the sentenceIf the package files are self-hosted, it is necessary to re-edit the Injects script in template permissions.
uses the fragment `#self-hosted-package-files` which does not match the section header “### Self-hosted package” (the fragment should be `#self-hosted-package`). Please update it to: ```diff - [self-hosted](#self-hosted-package-files) + [self-hosted](#self-hosted-package)to ensure consistency and valid linking.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
439-439: Link fragments should be valid
null(MD051, link-fragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)README.md(4 hunks)gtm_template.tpl(3 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
19-19: Unordered list style
Expected: dash; Actual: asterisk
(MD004, ul-style)
212-212: Fenced code blocks should have a language specified
null
(MD040, fenced-code-language)
225-225: Bare URL used
null
(MD034, no-bare-urls)
439-439: Link fragments should be valid
null
(MD051, link-fragments)
🪛 LanguageTool
README.md
[style] ~219-~219: Consider a more expressive alternative.
Context: ...le to host package files at any URL. To do this, the following steps are required:...
(DO_ACHIEVE)
[uncategorized] ~225-~225: A comma may be missing after the conjunctive/linking adverb ‘Also’.
Context: ...ww.example.com/static/cookie-consent/. Also the placeholder {version}` can used in...
(SENT_START_CONJUNCTIVE_LINKING_ADVERB_COMMA)
[grammar] ~226-~226: Did you mean “be used”?
Context: .... Also the placeholder {version}` can used in the field. Next, the GTM template n...
(WILL_BASED_ON)
[style] ~228-~228: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...d in the field. Next, the GTM template needs to be modified. In the "Permissions" tab, ...
(REP_NEED_TO_VB)
[style] ~230-~230: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...TM template is updated, the permissions need to be modified again, as an overwrite will...
(REP_NEED_TO_VB)
[grammar] ~230-~230: The verb ‘overwrite’ does not usually follow articles like ‘an’. Check that ‘overwrite’ is spelled correctly; using ‘overwrite’ as a noun may be non-standard.
Context: ...rmissions need to be modified again, as an overwrite will occur. <img src="docs/images/inje...
(A_INFINITIVE)
CHANGELOG.md
[duplication] ~9-~9: Possible typo: you repeated a word.
Context: ...spec/v2.0.0.html). ## [Unreleased] ### Added - Added new field "Package source" in the GTM t...
(ENGLISH_WORD_REPEAT_RULE)
🔇 Additional comments (5)
gtm_template.tpl (3)
48-54: Good implementation of the package source parameter.The addition of a
package_sourceparameter allows users to specify a custom URL for hosting package files, making it easy to use other CDNs or self-host the files. The help text clearly explains how to use this field and mentions the variable substitution capability.
418-423: Well-structured script URL construction logic.The implementation correctly handles:
- Using the custom package source if provided, otherwise defaulting to unpkg.com
- Replacing the version placeholder with the actual version value
- Ensuring the URL ends with a trailing slash for proper path construction
This provides flexibility while maintaining backward compatibility.
2779-2782: Appropriate permission added for the jsDelivr CDN.Adding permission to inject scripts from cdn.jsdelivr.net is necessary to support the jsDelivr CDN option mentioned in the PR objectives. This complements the package source parameter implementation.
README.md (2)
19-19: TOC Entry Updated for CDN/Self-Hosted Options
The new TOC entry* [Using other CDN or self-hosted](#using-other-cdn-or-self-hosted)provides clear navigation to the added section. Please verify that the fragment in the URL matches exactly with the corresponding header later in the document.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
19-19: Unordered list style
Expected: dash; Actual: asterisk(MD004, ul-style)
207-210: New Section: "Using other CDN or self-hosted" Added
The introduction of this new section is clear and well organized. It effectively outlines the options for both CDN hosting and self-hosting. Just ensure that the header text matches the TOC link for consistency.
tg666
force-pushed
the
feature/self-hosted
branch
from
fbf6255 to
3051918
Compare
2 participants
Summary by CodeRabbit
New Features
Documentation