fix: apply Prettier formatting and fix index.html HTML structure errors (closes #69)#72
Closed
Muneerali199 wants to merge 3 commits intoAOSSIE-Org:mainfrom
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughRefactors the SocialShareButton implementation to an event-driven lifecycle, adds analytics-related options, standardizes single-quote string style across docs and source, restructures and modernizes the demo/index.html with multiple demo sections and copy-to-clipboard UI, and updates integration docs and framework examples to match new patterns. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as Button/Modal (DOM)
participant Lib as SocialShareButton
participant Analytics
participant Clipboard
rect rgba(200,230,255,0.5)
User->>UI: Click share button
UI->>Lib: trigger open()
Lib->>UI: render modal / attach handlers
end
rect rgba(230,255,200,0.5)
User->>UI: Click platform action
UI->>Lib: platformClick(platform)
Lib->>Clipboard: build share URL / copy (if copy action)
Lib->>Analytics: emit analytics event (if enabled)
Lib->>UI: close modal (optional)
end
rect rgba(255,230,200,0.5)
Lib->>Lib: cleanup listeners on destroy
Lib->>Analytics: flush plugins / callbacks
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/social-share-button.js (1)
56-65:⚠️ Potential issue | 🔴 CriticalAvoid injecting
buttonTextthroughinnerHTML.
buttonTextis caller-controlled, and Line 64 renders it as raw HTML. Passing untrusted text here turns the button label into an XSS sink. Build the label node separately and assigntextContentinstead.🔒 Minimal hardening
- button.innerHTML = ` + button.innerHTML = ` <svg width="20" height="20" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg" class="share-icon"> <path d="M18 16.08C17.24 16.08 16.56 16.38 16.04 16.85L8.91 12.7C8.96 12.47 9 12.24 9 12C9 11.76 8.96 11.53 8.91 11.3L15.96 7.19C16.5 7.69 17.21 8 18 8C19.66 8 21 6.66 21 5C21 3.34 19.66 2 18 2C16.34 2 15 3.34 15 5C15 5.24 15.04 5.47 15.09 5.7L8.04 9.81C7.5 9.31 6.79 9 6 9C4.34 9 3 10.34 3 12C3 13.66 4.34 15 6 15C6.79 15 7.5 14.69 8.04 14.19L15.16 18.35C15.11 18.56 15.08 18.78 15.08 19C15.08 20.61 16.39 21.92 18 21.92C19.61 21.92 20.92 20.61 20.92 19C20.92 17.39 19.61 16.08 18 16.08Z" fill="currentColor"/> </svg> - <span>${this.options.buttonText}</span> + <span class="social-share-btn-label"></span> `; + button.querySelector('.social-share-btn-label').textContent = this.options.buttonText;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/social-share-button.js` around lines 56 - 65, The createButton method currently injects this.options.buttonText into button.innerHTML which allows caller-controlled text to be treated as HTML (XSS); fix by creating the button and SVG as before but do not interpolate buttonText into innerHTML—instead create a separate span element (e.g., const label = document.createElement('span')), set label.textContent = this.options.buttonText, and append it to the button (button.appendChild(label)); leave the static SVG markup either appended as a separate element or inserted safely (e.g., via a created element or innerHTML for the trusted SVG only) so that this.options.buttonText is never parsed as HTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/copilot/integrate-social-share-button.prompt.md:
- Around line 423-446: The docs and implementation are out of sync: update the
SocialShareButton constructor (and its defaultOptions / parseOptions helper) in
src/social-share-button.js to accept and store analytics, onAnalytics,
analyticsPlugins, componentId, and debug (with the documented defaults), and
either add 'email' to the implementation's default platforms array or change the
table to match the actual defaultPlatforms; ensure the new analytics-related
options are passed into the analytics emission code (or noop'd when analytics
=== false) so emitted events honor analytics, onAnalytics, analyticsPlugins,
componentId, and debug.
In `@index.html`:
- Around line 630-654: The copy button handler currently calls
navigator.clipboard.writeText(...) without a rejection handler, so add a .catch
handler on the promise returned by navigator.clipboard.writeText in the click
listener for copyButtons to handle failures: update the button text and the
nearby .copy-status span to a failure message, and implement a fallback that
creates a temporary textarea, sets its value to the text, selects it, runs
document.execCommand('copy') (or selects and instructs the user to press
Ctrl/Cmd+C), then cleans up the textarea; ensure the code in the catch restores
the original button text after a timeout just like the success path and
reference the existing variables button, codeBlock, text, and statusSpan when
implementing the fallback and failure UI updates.
- Around line 432-435: The React example imports the wrong package name; update
the import used by the App component so SocialShareButton resolves to the
published package: either switch to the CDN usage and reference
window.SocialShareButton in your demo, or change the ES import to import
SocialShareButton from
'social-share-button-aossie/src/social-share-button-react.jsx' so the
SocialShareButton symbol used in App and the onShare handler is correctly
imported.
In `@src/social-share-button.js`:
- Around line 371-385: updateOptions() currently only mutates this.options and
tweaks the URL input/colors, so UI can go out of sync when props like platforms,
buttonText, theme, buttonStyle, or modalPosition change; detect if any of those
keys are present in the incoming options and then trigger DOM updates (e.g.,
call methods such as this.renderButtons() to rebuild the button list,
this.renderModal() or this.render() to recreate modal contents, and
this.updateModalPosition() / this.applyTheme() / this.applyButtonStyle() as
appropriate) after merging options; if those helper methods don't exist,
implement concise helpers that re-render the button container and modal using
this.options so the visible widget always reflects the current options (keep
applyCustomColors call as-is).
- Around line 250-254: The anonymous Escape key handler added with
document.addEventListener should be stored so it can be removed during teardown:
create a named handler (e.g. this._onKeyDown or bind a class method) that checks
e.key === 'Escape' and this.isModalOpen and calls this.closeModal(), register
that handler with document.addEventListener, and remove it in destroy() via
document.removeEventListener using the same reference; update any initialization
path that currently attaches the anonymous listener to use the stored handler
instead.
---
Outside diff comments:
In `@src/social-share-button.js`:
- Around line 56-65: The createButton method currently injects
this.options.buttonText into button.innerHTML which allows caller-controlled
text to be treated as HTML (XSS); fix by creating the button and SVG as before
but do not interpolate buttonText into innerHTML—instead create a separate span
element (e.g., const label = document.createElement('span')), set
label.textContent = this.options.buttonText, and append it to the button
(button.appendChild(label)); leave the static SVG markup either appended as a
separate element or inserted safely (e.g., via a created element or innerHTML
for the trusted SVG only) so that this.options.buttonText is never parsed as
HTML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5090e835-e587-43a3-ba32-4430194f45bb
📒 Files selected for processing (6)
.github/copilot/integrate-social-share-button.prompt.mdREADME.mdindex.htmlsrc/social-share-button-react.jsxsrc/social-share-button.csssrc/social-share-button.js
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/social-share-button.js (1)
48-50:⚠️ Potential issue | 🟠 MajorSkip DOM initialization when
documentis unavailable.
constructor()still auto-runsinit()whenevercontaineris set. In SSR,new SocialShareButton({ container: '#share' })now crashes during render becausecreateButton()andcreateModal()immediately accessdocument.♻️ Proposed fix
- if (this.options.container) { + if (this.options.container && typeof document !== 'undefined') { this.init(); }Based on learnings, SSR compatibility is important for this public library and browser-only globals should be behind conditional code paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/social-share-button.js` around lines 48 - 50, The constructor currently calls this.init() when this.options.container is set, which breaks SSR because createButton() and createModal() access document; modify the constructor (or init entry) to guard DOM initialization by checking for browser environment (e.g., typeof document !== 'undefined' or typeof window !== 'undefined') before calling this.init(), or move the document-dependent logic inside init/createButton/createModal behind the same check so new SocialShareButton({ container: ... }) on the server does not attempt to access document.
🧹 Nitpick comments (1)
src/social-share-button.js (1)
8-497: Add descriptive comments to the touched methods.Most modified methods in this file still have no inline documentation, which makes the new lifecycle/share/update behavior harder to maintain. Please add brief comments for the constructor and touched methods explaining what they do, why they exist, and their parameters/return values.
Based on learnings, contributors to this repository want proper inline comments on modified methods in
src/social-share-button.js, especially comments covering purpose and parameters/return values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/social-share-button.js` around lines 8 - 497, The constructor and several methods (constructor, init, createButton, createModal, getPlatformsHTML, getShareURL, attachEvents, openModal, closeModal, share, copyLink, fallbackCopy, destroy, updateOptions, applyCustomColors) lack inline documentation; add brief JSDoc-style comments above each of these functions describing purpose, key parameters (e.g., options for constructor/updateOptions, platform for share/getShareURL), return behavior (if any), and any side-effects (e.g., DOM mutations, event registrations, clipboard use), keeping each comment concise (1–3 lines) to explain why the method exists and any important exceptions or lifecycle notes so future contributors can quickly understand usage and intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@index.html`:
- Around line 664-681: The call to navigator.clipboard.writeText can throw
synchronously in contexts where the Clipboard API is unavailable, preventing the
fallbackCopy path; update the code that currently calls
navigator.clipboard.writeText(text).then(showSuccess).catch(fallbackCopy) to
first check that navigator.clipboard and navigator.clipboard.writeText exist
(e.g., if (!navigator.clipboard || !navigator.clipboard.writeText) {
fallbackCopy(); return; }) and only call writeText when present, keeping
showSuccess/showFailure unchanged and ensuring fallbackCopy is invoked when the
API is missing or when the promise rejects.
In `@src/social-share-button.js`:
- Around line 184-203: The share message builders (whatsappMessage,
telegramMessage, emailBody and similar) currently append hardcoded promotional
copy like "Live on the site" and "Sharing a clean project I came across", which
mutates user-provided content; remove those hardcoded promo strings and build
defaults using only title, description and hashtagString (and the existing
punctuation/line breaks), and make any extra promotional copy opt-in via a new
explicit parameter (e.g., includePromo or promoText) passed into the public API
so callers can opt into adding phrases to whatsappMessage, telegramMessage or
emailBody rather than having them injected by default.
- Around line 267-285: openModal() and closeModal() currently leave opposing
timeouts uncleared and always reset document.body.style.overflow to '', causing
race conditions and clobbering pre-existing overflow; fix by tracking timeout
IDs (e.g., this._openTimeout and this._closeTimeout) and calling clearTimeout on
the opposite timer at the start of each method, canceling the pending callback,
and by saving the previous body overflow value (e.g.,
this._previousBodyOverflow) when opening and restoring that exact value when
closing (or removing the inline style if it was absent); also clear any pending
timeouts in the component's destroy/teardown path to avoid stray callbacks.
---
Outside diff comments:
In `@src/social-share-button.js`:
- Around line 48-50: The constructor currently calls this.init() when
this.options.container is set, which breaks SSR because createButton() and
createModal() access document; modify the constructor (or init entry) to guard
DOM initialization by checking for browser environment (e.g., typeof document
!== 'undefined' or typeof window !== 'undefined') before calling this.init(), or
move the document-dependent logic inside init/createButton/createModal behind
the same check so new SocialShareButton({ container: ... }) on the server does
not attempt to access document.
---
Nitpick comments:
In `@src/social-share-button.js`:
- Around line 8-497: The constructor and several methods (constructor, init,
createButton, createModal, getPlatformsHTML, getShareURL, attachEvents,
openModal, closeModal, share, copyLink, fallbackCopy, destroy, updateOptions,
applyCustomColors) lack inline documentation; add brief JSDoc-style comments
above each of these functions describing purpose, key parameters (e.g., options
for constructor/updateOptions, platform for share/getShareURL), return behavior
(if any), and any side-effects (e.g., DOM mutations, event registrations,
clipboard use), keeping each comment concise (1–3 lines) to explain why the
method exists and any important exceptions or lifecycle notes so future
contributors can quickly understand usage and intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 239b4d7e-dbfc-4271-83a6-26a1ca35b127
📒 Files selected for processing (2)
index.htmlsrc/social-share-button.js
Comment on lines
+664
to
+681
| function fallbackCopy() { | ||
| try { | ||
| const textarea = document.createElement('textarea'); | ||
| textarea.value = text; | ||
| textarea.style.position = 'fixed'; | ||
| textarea.style.opacity = '0'; | ||
| document.body.appendChild(textarea); | ||
| textarea.focus(); | ||
| textarea.select(); | ||
| document.execCommand('copy'); | ||
| document.body.removeChild(textarea); | ||
| showSuccess(); | ||
| } catch (_err) { | ||
| showFailure(); | ||
| } | ||
| } | ||
|
|
||
| navigator.clipboard.writeText(text).then(showSuccess).catch(fallbackCopy); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Is navigator.clipboard.writeText()guaranteed to exist in insecure contexts and older browsers, or should code feature-detectnavigator.clipboardbefore callingwriteText()?
💡 Result:
No—navigator.clipboard.writeText() is not guaranteed to exist in insecure contexts (http:) or older browsers.
- The Async Clipboard API is restricted to secure contexts;
navigator.clipboardmay be absent on non-HTTPS pages. The spec marksNavigator.clipboardas[SecureContext](so it’s not exposed in insecure contexts). [1][2][3] - Browser support is not universal (e.g., IE 11: not supported), so older browsers may not have
navigator.clipboard.writeText. [4]
So you should feature-detect (and typically also handle failures with try/catch), e.g.:
async function copyText(text) {
if (window.isSecureContext && navigator.clipboard?.writeText) {
await navigator.clipboard.writeText(text); // still may reject (e.g., no user gesture)
return;
}
// fallback (e.g., document.execCommand('copy') or UI prompt)
}Sources: [1] https://www.w3.org/TR/clipboard-apis/ [2] https://developer.mozilla.org/docs/Web/API/Clipboard_API [3] https://webkit.org/blog/10855/async-clipboard-api/ [4] https://caniuse.com/mdn-api_clipboard_writetext
Guard the Clipboard API before calling writeText().
navigator.clipboard is unavailable in insecure contexts (HTTP) and older browsers, so calling writeText() throws before the promise chain is created, making the fallback unreachable. Check for API availability first.
♻️ Proposed fix
- navigator.clipboard.writeText(text).then(showSuccess).catch(fallbackCopy);
+ if (navigator.clipboard && navigator.clipboard.writeText) {
+ navigator.clipboard.writeText(text).then(showSuccess).catch(fallbackCopy);
+ } else {
+ fallbackCopy();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function fallbackCopy() { | |
| try { | |
| const textarea = document.createElement('textarea'); | |
| textarea.value = text; | |
| textarea.style.position = 'fixed'; | |
| textarea.style.opacity = '0'; | |
| document.body.appendChild(textarea); | |
| textarea.focus(); | |
| textarea.select(); | |
| document.execCommand('copy'); | |
| document.body.removeChild(textarea); | |
| showSuccess(); | |
| } catch (_err) { | |
| showFailure(); | |
| } | |
| } | |
| navigator.clipboard.writeText(text).then(showSuccess).catch(fallbackCopy); | |
| function fallbackCopy() { | |
| try { | |
| const textarea = document.createElement('textarea'); | |
| textarea.value = text; | |
| textarea.style.position = 'fixed'; | |
| textarea.style.opacity = '0'; | |
| document.body.appendChild(textarea); | |
| textarea.focus(); | |
| textarea.select(); | |
| document.execCommand('copy'); | |
| document.body.removeChild(textarea); | |
| showSuccess(); | |
| } catch (_err) { | |
| showFailure(); | |
| } | |
| } | |
| if (navigator.clipboard && navigator.clipboard.writeText) { | |
| navigator.clipboard.writeText(text).then(showSuccess).catch(fallbackCopy); | |
| } else { | |
| fallbackCopy(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@index.html` around lines 664 - 681, The call to navigator.clipboard.writeText
can throw synchronously in contexts where the Clipboard API is unavailable,
preventing the fallbackCopy path; update the code that currently calls
navigator.clipboard.writeText(text).then(showSuccess).catch(fallbackCopy) to
first check that navigator.clipboard and navigator.clipboard.writeText exist
(e.g., if (!navigator.clipboard || !navigator.clipboard.writeText) {
fallbackCopy(); return; }) and only call writeText when present, keeping
showSuccess/showFailure unchanged and ensuring fallbackCopy is invoked when the
API is missing or when the promise rejects.
Comment on lines
184
to
+203
| // Build platform-specific messages with customizable parameters | ||
| let whatsappMessage, | ||
| facebookMessage, | ||
| twitterMessage, | ||
| telegramMessage, | ||
| redditTitle, | ||
| emailBody; | ||
| let whatsappMessage, facebookMessage, twitterMessage, telegramMessage, redditTitle, emailBody; | ||
|
|
||
| // WhatsApp: Casual with emoji | ||
| whatsappMessage = `\u{1F680} ${title}${description ? "\n\n" + description : ""}${hashtagString ? "\n\n" + hashtagString : ""}\n\nLive on the site \u{1F440}\nClean UI, smooth flow \u{2014} worth peeking\n\u{1F447}`; | ||
| whatsappMessage = `\u{1F680} ${title}${description ? '\n\n' + description : ''}${hashtagString ? '\n\n' + hashtagString : ''}\n\nLive on the site \u{1F440}\nClean UI, smooth flow \u{2014} worth peeking\n\u{1F447}`; | ||
|
|
||
| // Facebook: Title + Description | ||
| facebookMessage = `${title}${description ? "\n\n" + description : ""}${hashtagString ? "\n\n" + hashtagString : ""}`; | ||
| facebookMessage = `${title}${description ? '\n\n' + description : ''}${hashtagString ? '\n\n' + hashtagString : ''}`; | ||
|
|
||
| // Twitter: Title + Description + Hashtags + Via | ||
| twitterMessage = `${title}${description ? "\n\n" + description : ""}${hashtagString ? "\n" + hashtagString : ""}`; | ||
| twitterMessage = `${title}${description ? '\n\n' + description : ''}${hashtagString ? '\n' + hashtagString : ''}`; | ||
|
|
||
| // Telegram: Casual with emoji | ||
| telegramMessage = `\u{1F517} ${title}${description ? "\n\n" + description : ""}${hashtagString ? "\n\n" + hashtagString : ""}\n\nLive + working\nClean stuff, take a look \u{1F447}`; | ||
| telegramMessage = `\u{1F517} ${title}${description ? '\n\n' + description : ''}${hashtagString ? '\n\n' + hashtagString : ''}\n\nLive + working\nClean stuff, take a look \u{1F447}`; | ||
|
|
||
| // Reddit: Title + Description | ||
| redditTitle = `${title}${description ? " - " + description : ""}`; | ||
| redditTitle = `${title}${description ? ' - ' + description : ''}`; | ||
|
|
||
| // Email: Friendly greeting | ||
| emailBody = `Hey \u{1F44B}\n\nSharing a clean project I came across:\n${title}${description ? "\n\n" + description : ""}\n\nLive, simple, and usable \u{2014} take a look \u{1F447}`; | ||
| emailBody = `Hey \u{1F44B}\n\nSharing a clean project I came across:\n${title}${description ? '\n\n' + description : ''}\n\nLive, simple, and usable \u{2014} take a look \u{1F447}`; |
Contributor
There was a problem hiding this comment.
Keep the generated share text user-supplied.
These defaults now append library-specific promo copy like “Live on the site” and “Sharing a clean project I came across” to WhatsApp, Telegram, and Email shares. That changes the caller’s content unexpectedly for every consumer of the library. Default payloads should be built from title / description / hashtags only, with any extra copy made opt-in.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/social-share-button.js` around lines 184 - 203, The share message
builders (whatsappMessage, telegramMessage, emailBody and similar) currently
append hardcoded promotional copy like "Live on the site" and "Sharing a clean
project I came across", which mutates user-provided content; remove those
hardcoded promo strings and build defaults using only title, description and
hashtagString (and the existing punctuation/line breaks), and make any extra
promotional copy opt-in via a new explicit parameter (e.g., includePromo or
promoText) passed into the public API so callers can opt into adding phrases to
whatsappMessage, telegramMessage or emailBody rather than having them injected
by default.
Comment on lines
267
to
285
| openModal() { | ||
| // Safety check: prevent errors if modal was destroyed | ||
| if (!this.modal) return; | ||
|
|
||
| this.isModalOpen = true; | ||
| this.modal.style.display = "flex"; | ||
|
|
||
| // Shared body overflow management: only increment counter if this instance doesn't already own the lock | ||
| if (typeof document !== "undefined" && document.body) { | ||
| if (!this.ownsBodyLock) { | ||
| // Only increment if this instance doesn't already own a lock | ||
| if (SocialShareButton.openModalCount === 0) { | ||
| // Save original overflow before first modal opens | ||
| SocialShareButton.originalBodyOverflow = document.body.style.overflow; | ||
| } | ||
| SocialShareButton.openModalCount++; | ||
| this.ownsBodyLock = true; // Mark that this instance owns a lock | ||
| } | ||
| document.body.style.overflow = "hidden"; | ||
| } | ||
|
|
||
| // Clear any pending animations (both open and close to prevent race conditions) | ||
| if (this.openTimeout) { | ||
| clearTimeout(this.openTimeout); | ||
| this.openTimeout = null; | ||
| } | ||
| if (this.closeTimeout) { | ||
| clearTimeout(this.closeTimeout); | ||
| this.closeTimeout = null; | ||
| } | ||
| this.modal.style.display = 'flex'; | ||
| document.body.style.overflow = 'hidden'; | ||
|
|
||
| // Animate in | ||
| this.openTimeout = setTimeout(() => { | ||
| if (this.modal) { // Safety check in case destroy() was called | ||
| this.modal.classList.add("active"); | ||
| } | ||
| this.openTimeout = null; | ||
| setTimeout(() => { | ||
| this.modal.classList.add('active'); | ||
| }, 10); | ||
| } | ||
|
|
||
| closeModal() { | ||
| if (!this.modal) return; // Safety check | ||
|
|
||
| this.modal.classList.remove("active"); | ||
|
|
||
| // Clear any pending animations (both open and close to prevent race conditions) | ||
| if (this.openTimeout) { | ||
| clearTimeout(this.openTimeout); | ||
| this.openTimeout = null; | ||
| } | ||
| if (this.closeTimeout) { | ||
| clearTimeout(this.closeTimeout); | ||
| this.closeTimeout = null; | ||
| } | ||
| this.modal.classList.remove('active'); | ||
|
|
||
| this.closeTimeout = setTimeout(() => { | ||
| if (this.modal) { // Safety check in case destroy() was called | ||
| this.isModalOpen = false; | ||
| this.modal.style.display = "none"; | ||
|
|
||
| // Shared body overflow management: only decrement if this instance owns the lock | ||
| if (this.ownsBodyLock && typeof document !== "undefined" && document.body) { | ||
| // Decrement counter (guard against negative) | ||
| if (SocialShareButton.openModalCount > 0) { | ||
| SocialShareButton.openModalCount--; | ||
| } | ||
| this.ownsBodyLock = false; // Release the lock | ||
|
|
||
| // Restore original overflow only when all modals are closed | ||
| if (SocialShareButton.openModalCount === 0) { | ||
| document.body.style.overflow = SocialShareButton.originalBodyOverflow || ""; | ||
| SocialShareButton.originalBodyOverflow = null; | ||
| } | ||
| } | ||
| } | ||
| this.closeTimeout = null; | ||
| setTimeout(() => { | ||
| this.isModalOpen = false; | ||
| this.modal.style.display = 'none'; | ||
| document.body.style.overflow = ''; | ||
| }, 200); |
Contributor
There was a problem hiding this comment.
Clear the transition timers and restore the previous body overflow value.
openModal() and closeModal() both enqueue timeouts, but neither clears the opposite one. Reopening inside the 200ms close window leaves a stale callback that hides the reopened modal and unlocks body scroll at the wrong time. Also, closing/destroying always writes '', which clobbers any pre-existing inline body.style.overflow.
♻️ Proposed fix
openModal() {
+ clearTimeout(this._closeTimer);
this.isModalOpen = true;
this.modal.style.display = 'flex';
- document.body.style.overflow = 'hidden';
+ this._previousBodyOverflow ??= document.body.style.overflow;
+ document.body.style.overflow = 'hidden';
- setTimeout(() => {
+ this._openTimer = setTimeout(() => {
this.modal.classList.add('active');
}, 10);
}
closeModal() {
+ clearTimeout(this._openTimer);
this.modal.classList.remove('active');
- setTimeout(() => {
+ this._closeTimer = setTimeout(() => {
this.isModalOpen = false;
this.modal.style.display = 'none';
- document.body.style.overflow = '';
+ document.body.style.overflow = this._previousBodyOverflow || '';
+ this._previousBodyOverflow = null;
}, 200);
}Also applies to: 383-383
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/social-share-button.js` around lines 267 - 285, openModal() and
closeModal() currently leave opposing timeouts uncleared and always reset
document.body.style.overflow to '', causing race conditions and clobbering
pre-existing overflow; fix by tracking timeout IDs (e.g., this._openTimeout and
this._closeTimeout) and calling clearTimeout on the opposite timer at the start
of each method, canceling the pending callback, and by saving the previous body
overflow value (e.g., this._previousBodyOverflow) when opening and restoring
that exact value when closing (or removing the inline style if it was absent);
also clear any pending timeouts in the component's destroy/teardown path to
avoid stray callbacks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR enforces the
.prettierrc.jsonsingleQuote: truerule consistently across all source files, resolving the conflict between Prettier defaults (double quotes) and the ESLint\"quotes\": [\"error\", \"single\"]rule. It also fixes HTML structure errors inindex.htmland addresses all CodeRabbit review findings.Related Issue
Closes #69
Screenshots/Video (if applicable)
N/A
Testing
npx prettier --checkpasses on all changed filesnpx eslint src/social-share-button.js— 0 errorsChecklist