reshaped to fit ghost-theme#4
Conversation
There was a problem hiding this comment.
Pull request overview
This PR replaces the existing Tingle.js-based modal with a custom CSS overlay + iframe approach to better match the ghost-theme / react-timely-plugin layout, and updates packaging to support Yarn + an esbuild IIFE build.
Changes:
- Replaced
tingle.jsmodal implementation with a custom overlay + responsive iframe modal and postMessage-based close. - Updated README usage docs (bundler + script-tag/IIFE) and added responsive layout notes.
- Migrated from npm lockfile to Yarn (Berry, node-modules linker) and added an esbuild
build:iifescript; bumped version to1.0.3.
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
index.js |
Implements the new overlay/iframe modal, responsive CSS, and postMessage close handling. |
README.md |
Updates API/usage documentation and adds IIFE/script-tag usage instructions. |
package.json |
Removes runtime dependency, adds build:iife script, bumps version, adds esbuild devDependency. |
package-lock.json |
Removes npm lockfile as part of Yarn migration. |
yarn.lock |
Adds Yarn lockfile for the new dependency set (esbuild). |
.yarnrc.yml |
Configures Yarn nodeLinker: node-modules. |
.gitignore |
Ignores dist/ and Yarn install state. |
Comments suppressed due to low confidence (1)
index.js:108
closeMethodsis still part of the exportedopenTimely()signature but is no longer used anywhere in the implementation. This is now an API/documentation mismatch (README describes only 3 params) and can confuse callers; either removecloseMethodsfrom the signature (major version bump) or reintroduce behavior that uses it.
export function openTimely(
eventName,
params = {},
closeMethods = [],
env = "prod"
) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const timelyOverlay = document.createElement("div"); | ||
| timelyOverlay.className = "timely-overlay"; | ||
| document.body.appendChild(timelyOverlay); | ||
|
|
There was a problem hiding this comment.
document.body.appendChild(timelyOverlay) runs at module load time and will throw if this script is loaded before <body> exists (e.g., included in <head>). Consider lazily creating/appending the overlay inside openTimely() (or gating behind DOMContentLoaded) to avoid hard dependency on document.body being available during import.
| const timelyUrlStaging = `https://test-timely.joinditto.in/event/${eventName}/book`; | ||
| const timelyUrlProd = `https://timely.joinditto.in/event/${eventName}/book`; |
There was a problem hiding this comment.
eventName is interpolated directly into the URL path without encoding. If it contains characters like /, ?, #, or quotes, the resulting src can be malformed and (because it's later inserted into HTML) can become an injection vector. Build the URL with encodeURIComponent(eventName) (or the URL API) before setting the iframe src.
| const timelyUrlStaging = `https://test-timely.joinditto.in/event/${eventName}/book`; | |
| const timelyUrlProd = `https://timely.joinditto.in/event/${eventName}/book`; | |
| const encodedEventName = encodeURIComponent(eventName); | |
| const timelyUrlStaging = `https://test-timely.joinditto.in/event/${encodedEventName}/book`; | |
| const timelyUrlProd = `https://timely.joinditto.in/event/${encodedEventName}/book`; |
| const timelyUrlWithParams = `${timelyUrl}?${queryString}`; | ||
|
|
||
| timelyOverlay.innerHTML = ` | ||
| <iframe id="timely-iframe" class="timely-iframe" src="${timelyUrlWithParams}"></iframe> | ||
| `; |
There was a problem hiding this comment.
The iframe is created via timelyOverlay.innerHTML with a src built from runtime values. Even with encodeURIComponent for query params, HTML injection is still possible via the unencoded eventName path, and using innerHTML is unnecessarily risky. Prefer creating the iframe with document.createElement('iframe') and setting iframe.src/className/id directly.
| function timelyMessageListener(event) { | ||
| const { data } = event; | ||
| if (data.from === "timely" && data.action === "confirm-close") { | ||
| closeTimely(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The message handler accepts { from: "timely", action: "confirm-close" } from any origin/source, so any script on the host page can close the modal by posting that payload. Validate event.origin (prod/test Timely domains) and ideally also event.source === document.getElementById('timely-iframe')?.contentWindow before acting on the message.
| timelyModal.close(); | ||
| timelyOverlay.classList.remove("timely-overlay--visible"); | ||
| document.documentElement.classList.remove("timely-open"); | ||
| window.removeEventListener("message", timelyMessageListener); |
There was a problem hiding this comment.
closeTimely() hides the overlay but leaves the iframe running in the DOM. This can keep network activity/timers alive and retain memory even after closing. Consider clearing/removing the iframe node on close (e.g., emptying timelyOverlay or removing/recreating the overlay) so closing actually tears down the embedded page.
| window.removeEventListener("message", timelyMessageListener); | |
| window.removeEventListener("message", timelyMessageListener); | |
| // Clear iframe content so the embedded page is fully torn down | |
| timelyOverlay.innerHTML = ""; |
| The `openTimely` function takes 3 parameters. | ||
|
|
||
| 1.`eventName` which is mandatory is used to send the name of the timely event. It would be a string value. | ||
| 1. `eventName` which is mandatory is used to send the name of the timely event. It would be a string value. | ||
|
|
||
| 2.`params` is an optional field. It is an object, which can be used to send the **utm_params** and any other params which is mutually agreed with Ditto tech team. | ||
| 2. `params` is an optional field. It is an object, which can be used to send the **utm_params** and any other params which is mutually agreed with Ditto tech team. | ||
|
|
||
| 3.`closeMethods` is an optional field.It is an array of strings with possible values as **overlay** or **button**. | ||
| By default all the 2 methods are enabled.If you want only certain close methods pass only those. | ||
| **button** - Modal is closed on clicking close button. | ||
| **overlay** -Modal is closed on clicking the overlay. | ||
|
|
||
| 4.`env` which is used specify the timely server (prod/staging).It would be a string value.The values can be either **prod** or **staging**. By default it is set to **prod**. | ||
| 3. `env` which is used to specify the timely server (prod/staging). It would be a string value. The values can be either **prod** or **staging**. By default it is set to **prod**. | ||
|
|
There was a problem hiding this comment.
README states openTimely takes 3 parameters (eventName, params, env), but the actual exported function signature still includes a 3rd closeMethods parameter before env. Update the docs to match the current API, or adjust the API to match the documented 3-parameter form.
| "scripts": { | ||
| "build:iife": "esbuild index.js --bundle --format=iife --global-name=DittoTimely --outfile=dist/vanilla-timely.iife.js --minify" | ||
| }, | ||
| "publishConfig": { | ||
| "access": "public" | ||
| }, | ||
| "devDependencies": { | ||
| "esbuild": "^0.27.4" | ||
| } |
There was a problem hiding this comment.
build:iife generates dist/vanilla-timely.iife.js, but nothing ensures it’s built before publishing to npm. Since the README documents script-tag usage from dist/, consider adding a prepack/prepublishOnly script (or publishing dist via the files field) so consumers reliably get the IIFE artifact.
| @@ -1,154 +1,149 @@ | |||
| import Tingle from "tingle.js"; | |||
| // --- CSS (matches react-timely-plugin's TimelyModal.tsx) --- | |||
There was a problem hiding this comment.
can remove this comment
Summary
tingle.jsdependency and replaced the modal implementation with a custom CSS overlay + iframe approach, matching the layout fromreact-timely-plugin'sTimelyModal.tsxembed_type,embed_domain, andembed_pathquery params so the iframe handles its own close button via postMessagepackage-lock.jsonto Yarn withnodeLinker: node-modules, and added anesbuildIIFE build script (build:iife)1.0.3Test plan
yarn build:iifeand verifydist/vanilla-timely.iife.jsis generated