Enhance SocialShareButton with customizable open/close callbacks and …#127
Open
sarthakdev14 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Open
Enhance SocialShareButton with customizable open/close callbacks and …#127sarthakdev14 wants to merge 2 commits intoAOSSIE-Org:mainfrom
sarthakdev14 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
…improved accessibility attributes
Contributor
WalkthroughAdds lifecycle callbacks ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/social-share-button.js (2)
106-114:⚠️ Potential issue | 🟡 MinorAdd accessible name to dialog via
aria-labelledby.The dialog has
role="dialog"andaria-modal="true", but lacks an accessible name. Screen readers needaria-labelledbyreferencing the heading to announce the dialog's purpose.♿ Proposed fix
const modal = document.createElement("div"); modal.setAttribute("role", "dialog"); modal.setAttribute("aria-modal", "true"); modal.setAttribute("aria-hidden", "true"); + modal.setAttribute("aria-labelledby", "social-share-modal-title"); modal.className = `social-share-modal-overlay ${this.options.theme}`; modal.style.display = "none"; modal.innerHTML = ` <div class="social-share-modal-content ${this.options.modalPosition}"> <div class="social-share-modal-header"> - <h3>Share</h3> + <h3 id="social-share-modal-title">Share</h3>Note: If multiple instances exist on the same page, you'll need unique IDs (e.g., append
this.options.componentIdor a counter).🤖 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 106 - 114, The dialog created in the modal block lacks an accessible name; add an id to the heading inside the template and set modal.setAttribute("aria-labelledby", headingId) so screen readers announce the dialog; generate a unique headingId (e.g., `social-share-title-${this.options.componentId || uniqueCounter}`) and inject that id into the <h3> element in the template string (and then call modal.setAttribute("aria-labelledby", headingId)) in the same initialization code that creates `modal` (referencing the modal variable and the template that contains the <h3> "Share").
201-235:⚠️ Potential issue | 🟡 MinorInconsistent
shareTextapplication across platforms.The
messagevariable (derived fromshareText) is only used for WhatsApp, Facebook, and Twitter. Telegram, Reddit, Email, and Pinterest still referencetitledirectly. This partial implementation may confuse API consumers expectingshareTextto customize all platforms uniformly.Consider applying
messageconsistently, or document which platforms supportshareText.🔧 Suggested fix for consistency
// 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} ${message}${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 = `${message}${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${message}${description ? "\n\n" + description : ""}\n\nLive, simple, and usable \u{2014} take a look \u{1F447}`; // Pinterest: Title + Description - pinterestText = `${title || ""}${description ? " - " + description : ""}`; + pinterestText = `${message}${description ? " - " + description : ""}`;🤖 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 201 - 235, The issue: shareText (message) is only applied to WhatsApp/Facebook/Twitter while Telegram, Reddit, Email and Pinterest still use title directly; update platform messages to use the unified message variable (message) where appropriate. Locate telegramMessage, redditTitle, emailBody, and pinterestText and replace their direct uses of title with message (keeping the existing description and hashtagString logic and emojis intact), and ensure message still falls back to title or a default as defined by const message = shareText || title || "Check this out".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/social-share-button.js`:
- Line 202: The hardcoded fallback "Check this out" should be made configurable
for i18n: add a new option/prop named defaultShareText and use it when computing
message (replace the expression that sets const message = shareText || title ||
"Check this out" with shareText || title || defaultShareText || "Check this
out"); update the component/function signature or options object to accept
defaultShareText (and provide a sensible default in defaults/propTypes or
equivalent) and update any docs/readme to mention developers can set
defaultShareText for localization.
- Around line 34-36: Add brief inline comments for the new public options in the
options object so callers understand their purpose: add one-line comments next
to onOpen, onClose, and shareText in the options initializer (e.g., where
onOpen: options.onOpen || null, onClose: options.onClose || null, shareText:
options.shareText || "") describing that onOpen/onClose are optional callback
hooks invoked when the share UI opens/closes and shareText is the default text
to prefill for sharing; keep comments minimal and factual to follow repository
guidelines.
---
Outside diff comments:
In `@src/social-share-button.js`:
- Around line 106-114: The dialog created in the modal block lacks an accessible
name; add an id to the heading inside the template and set
modal.setAttribute("aria-labelledby", headingId) so screen readers announce the
dialog; generate a unique headingId (e.g.,
`social-share-title-${this.options.componentId || uniqueCounter}`) and inject
that id into the <h3> element in the template string (and then call
modal.setAttribute("aria-labelledby", headingId)) in the same initialization
code that creates `modal` (referencing the modal variable and the template that
contains the <h3> "Share").
- Around line 201-235: The issue: shareText (message) is only applied to
WhatsApp/Facebook/Twitter while Telegram, Reddit, Email and Pinterest still use
title directly; update platform messages to use the unified message variable
(message) where appropriate. Locate telegramMessage, redditTitle, emailBody, and
pinterestText and replace their direct uses of title with message (keeping the
existing description and hashtagString logic and emojis intact), and ensure
message still falls back to title or a default as defined by const message =
shareText || title || "Check this out".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7d6c56c-ecc3-4078-958c-a49e7dcccb10
📒 Files selected for processing (1)
src/social-share-button.js
…ibility attributes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
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)
117-125:⚠️ Potential issue | 🟡 MinorMissing
type="button"on modal buttons.The close button (line 117) and copy button (line 125) are missing the explicit
type="button"attribute, inconsistent with the main share button and platform buttons that were updated.🔧 Proposed fix
- <button class="social-share-modal-close" aria-label="Close">✕</button> + <button type="button" class="social-share-modal-close" aria-label="Close">✕</button>- <button class="social-share-copy-btn">Copy</button> + <button type="button" class="social-share-copy-btn">Copy</button>🤖 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 117 - 125, The modal close and copy buttons are missing explicit type attributes; update the button elements with class names social-share-modal-close and social-share-copy-btn to include type="button" so they don't act as default submit buttons (aligning with the other buttons and platform buttons generated by getPlatformsHTML()); locate these elements in src/social-share-button.js and add the attribute to each button tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/social-share-button.js`:
- Around line 340-346: Add a short inline comment above the try/catch that
explains why we guard the user-provided callback: the block around
this.options.onOpen({ url: this.options.url, platform: null }) should note that
we call a third‑party/user callback so we catch exceptions to prevent them from
bubbling up and to log via this._debugWarn; mention that this is a defensive
safety check for faulty callbacks.
- Around line 107-116: The modal title ID is hardcoded as
"social-share-modal-title", causing duplicate IDs across multiple
SocialShareButton instances; modify createModal() to generate a unique id per
instance (e.g., using an instance-specific identifier like this.instanceId, a
counter, or timestamp), assign that unique id to the title element instead of
the hardcoded string, and update the modal's aria-labelledby attribute to
reference this generated id so the accessibility association remains correct for
each instance.
---
Outside diff comments:
In `@src/social-share-button.js`:
- Around line 117-125: The modal close and copy buttons are missing explicit
type attributes; update the button elements with class names
social-share-modal-close and social-share-copy-btn to include type="button" so
they don't act as default submit buttons (aligning with the other buttons and
platform buttons generated by getPlatformsHTML()); locate these elements in
src/social-share-button.js and add the attribute to each button tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90314f37-d7e8-4993-9310-02dfb2e1b2f2
📒 Files selected for processing (1)
src/social-share-button.js
Comment on lines
+107
to
+116
| modal.setAttribute("role", "dialog"); | ||
| modal.setAttribute("aria-modal", "true"); | ||
| modal.setAttribute("aria-labelledby", "social-share-modal-title"); | ||
| modal.setAttribute("aria-hidden", "true"); | ||
| modal.className = `social-share-modal-overlay ${this.options.theme}`; | ||
| modal.style.display = "none"; | ||
| modal.innerHTML = ` | ||
| <div class="social-share-modal-content ${this.options.modalPosition}"> | ||
| <div class="social-share-modal-header"> | ||
| <h3>Share</h3> | ||
| <h3 id="social-share-modal-title">Share</h3> |
Contributor
There was a problem hiding this comment.
Duplicate ID issue with multiple component instances.
The hardcoded id="social-share-modal-title" will cause duplicate IDs when multiple SocialShareButton instances exist on the same page. This breaks the aria-labelledby association and violates HTML validity.
Consider using a unique identifier per instance:
🛠️ Proposed fix
+ // Generate unique ID for this instance (used for ARIA associations)
+ this.instanceId = `ssb-${Date.now()}-${Math.random().toString(36).slice(2, 9)}`;Then in createModal():
- modal.setAttribute("aria-labelledby", "social-share-modal-title");
+ modal.setAttribute("aria-labelledby", `social-share-modal-title-${this.instanceId}`);- <h3 id="social-share-modal-title">Share</h3>
+ <h3 id="social-share-modal-title-${this.instanceId}">Share</h3>🤖 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 107 - 116, The modal title ID is
hardcoded as "social-share-modal-title", causing duplicate IDs across multiple
SocialShareButton instances; modify createModal() to generate a unique id per
instance (e.g., using an instance-specific identifier like this.instanceId, a
counter, or timestamp), assign that unique id to the title element instead of
the hardcoded string, and update the modal's aria-labelledby attribute to
reference this generated id so the accessibility association remains correct for
each instance.
Comment on lines
+340
to
+346
| if (typeof this.options.onOpen === "function") { | ||
| try { | ||
| this.options.onOpen({ url: this.options.url, platform: null }); | ||
| } catch (err) { | ||
| this._debugWarn("onOpen callback failed", err); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add brief inline comment for the callback guard.
Per repository guidelines, non-obvious logic like try/catch guards should have brief comments. Consider:
+ // Invoke lifecycle callback safely; errors are non-fatal
if (typeof this.options.onOpen === "function") {
try {
this.options.onOpen({ url: this.options.url, platform: null });
} catch (err) {
this._debugWarn("onOpen callback failed", err);
}
}As per coding guidelines: "Add comments for edge cases and non-obvious logic (safety checks, race condition prevention, re-entrancy guards, cleanup callbacks, etc.)"
🤖 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 340 - 346, Add a short inline
comment above the try/catch that explains why we guard the user-provided
callback: the block around this.options.onOpen({ url: this.options.url,
platform: null }) should note that we call a third‑party/user callback so we
catch exceptions to prevent them from bubbling up and to log via
this._debugWarn; mention that this is a defensive safety check for faulty
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.
🚀 PR: SocialShareButton – Usability, Accessibility & API Improvements
📌 Summary
This PR improves
social-share-button.jsby enhancing usability, accessibility, and API flexibility while maintaining backward compatibility.✨ What’s Changed
shareTextfor better share content fallbackonOpenandonClosecallbacksonShare,onCopy, and analytics flow (social_share_*events, DOM events, plugin adapters)♿ Accessibility Improvements
aria-hiddentoggling on open/close⚙️ Behavior Improvements
📊 Status
get_errorsclean)🧪 Next Steps
💡 Impact
Improves developer experience, accessibility compliance, and overall reliability of the library.
Summary by CodeRabbit
New Features
Bug Fixes / Accessibility