Fix: improved UI and accessibility of social share button#128
Open
Sneh30 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Open
Fix: improved UI and accessibility of social share button#128Sneh30 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Sneh30 wants to merge 1 commit intoAOSSIE-Org:mainfrom
Conversation
github-actions
bot
added
no-issue-linked
ci-cd
documentation
Contributor
WalkthroughThis PR introduces comprehensive built-in analytics capabilities to SocialShareButton, adding runtime configuration options, event history tracking, event ID generation, and two new built-in adapters for Google Tag Manager and console logging integration. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 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)
.github/copilot/integrate-analytics.prompt.md (1)
103-110:⚠️ Potential issue | 🟡 MinorCDN URL uses
@mainbranch instead of version tag.Line 106 references
@mainwhich points to the development branch. This may expose users to unreleased/unstable code. Consider using a specific version tag (e.g.,@v1.0.4) once this feature is released.Per retrieved learnings: "new wrapper files added in a PR are not part of any CDN release until the next version tag is cut."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/copilot/integrate-analytics.prompt.md around lines 103 - 110, Update the CDN reference that uses the unstable branch by replacing the "@main" tag in the social-share-analytics.js script URL with a specific release tag (e.g., "@v1.0.4") and update the accompanying comment to instruct using a versioned tag; also audit the alternate npm import line ("social-share-button-aossie/src/social-share-analytics.js") to ensure it references the published package name/path for the same released version so the docs and the script tag consistently point to a released artifact instead of the main branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 123-145: The README example references ConsoleAdapter but doesn't
show how to obtain or import it, causing a ReferenceError; update the snippet to
either (a) show the import/registration for ConsoleAdapter (e.g., an import or
adapter factory call) before new SocialShareButton(...) so ConsoleAdapter is
defined, or (b) add a short note and link to the analytics integration docs
explaining how to load ConsoleAdapter; reference the symbols ConsoleAdapter,
SocialShareButton, and analyticsPlugins so readers can see where to add the
adapter setup.
In `@src/social-share-analytics.js`:
- Around line 249-267: Add a brief inline comment above the
GoogleTagManagerAdapter.track method describing its purpose and behavior: state
that it guards against server-side execution (checks typeof window), ensures
window.dataLayer is an array, and then pushes a standardized analytics payload
to the GTM dataLayer, including optional conditional spreads for errorMessage
and context; update the comment to mention why the conditional spread operators
are used to avoid adding undefined fields.
In `@src/social-share-button.js`:
- Around line 784-792: Add a brief inline comment above the sampling conditional
that explains the sampling behavior in plain terms: compute sampleRate from
analyticsOptions.sampleRate (defaults to 1), treat any value > 1 as 100% (no
sampling), treat values between 0 and 1 as probabilistic sampling using
Math.random(), and short-circuit when sampleRate <= 0; also note that the
subsequent analyticsOptions.filter(payload) is an independent filter step. Place
this comment near the sampleRate declaration/conditional so readers of
analyticsOptions, sampleRate, and the payload/filter logic immediately
understand the edge cases.
- Around line 803-811: The current Array branch for analyticsOptions.dedupeBy
uses every() which returns false if any key is missing, silently disabling
dedupe; update the logic in src/social-share-button.js where sameEvent is set
for analyticsOptions.dedupeBy so that it first filters the dedupe keys to those
actually present on both payload and this._lastAnalyticsEvent (or alternatively
log a debug/warning for missing keys) and then compare only those existing keys
for equality; ensure you still handle the case where no valid keys remain (e.g.,
treat as not-duplicate or log) so deduplication behavior is explicit.
- Around line 695-701: Add a brief inline comment for the _generateEventId()
function that explains the event ID format and purpose: note that it returns a
base36 timestamp followed by a hyphen and an 8-character base36 random string
(timestamp for ordering + random suffix for uniqueness), and place the comment
immediately above the function declaration (not as JSDoc) so readers quickly
understand the ID structure used for emitted analytics events.
---
Outside diff comments:
In @.github/copilot/integrate-analytics.prompt.md:
- Around line 103-110: Update the CDN reference that uses the unstable branch by
replacing the "@main" tag in the social-share-analytics.js script URL with a
specific release tag (e.g., "@v1.0.4") and update the accompanying comment to
instruct using a versioned tag; also audit the alternate npm import line
("social-share-button-aossie/src/social-share-analytics.js") to ensure it
references the published package name/path for the same released version so the
docs and the script tag consistently point to a released artifact instead of the
main branch.
🪄 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: c22be96c-71e9-40a3-a3cb-81c9bd68ee02
📒 Files selected for processing (5)
.github/copilot/integrate-analytics.prompt.md.github/copilot/integrate-social-share-button.prompt.mdREADME.mdsrc/social-share-analytics.jssrc/social-share-button.js
Comment on lines
+123
to
+145
| ```js | ||
| const shareButton = new SocialShareButton({ | ||
| container: "#share-button", | ||
| analyticsOptions: { | ||
| sampleRate: 0.6, // keep volume down in load tests | ||
| dedupeWindow: 3000, // suppress duplicate clicks in short windows | ||
| dedupeBy: ["eventName", "platform","url"], | ||
| historyLimit: 150, | ||
| enrichContext: true, | ||
| includeUserAgent: false, | ||
| }, | ||
| analyticsPlugins: [new ConsoleAdapter()], | ||
| onAnalytics: (payload) => { | ||
| // custom filtering + forwarding | ||
| if (payload.eventName !== "social_share_popup_open") { | ||
| sendTelemetry(payload); | ||
| } | ||
| }, | ||
| }); | ||
|
|
||
| console.log(shareButton.getAnalyticsHistory()); | ||
| shareButton.clearAnalyticsHistory(); | ||
| ``` |
Contributor
There was a problem hiding this comment.
Example is incomplete — ConsoleAdapter usage not explained.
The code snippet uses new ConsoleAdapter() directly, but doesn't show how to load or access it. Users following this example will get a ReferenceError.
Consider adding the required setup, similar to how it's documented in the analytics prompt file:
+// Load the analytics adapters script first, then:
+const { ConsoleAdapter } = window.SocialShareAnalytics;
+
const shareButton = new SocialShareButton({
container: "#share-button",
analyticsOptions: {
sampleRate: 0.6,
// ...
},
- analyticsPlugins: [new ConsoleAdapter()],
+ analyticsPlugins: [new ConsoleAdapter()],
// ...
});Alternatively, link to the detailed analytics integration documentation.
📝 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
| ```js | |
| const shareButton = new SocialShareButton({ | |
| container: "#share-button", | |
| analyticsOptions: { | |
| sampleRate: 0.6, // keep volume down in load tests | |
| dedupeWindow: 3000, // suppress duplicate clicks in short windows | |
| dedupeBy: ["eventName", "platform","url"], | |
| historyLimit: 150, | |
| enrichContext: true, | |
| includeUserAgent: false, | |
| }, | |
| analyticsPlugins: [new ConsoleAdapter()], | |
| onAnalytics: (payload) => { | |
| // custom filtering + forwarding | |
| if (payload.eventName !== "social_share_popup_open") { | |
| sendTelemetry(payload); | |
| } | |
| }, | |
| }); | |
| console.log(shareButton.getAnalyticsHistory()); | |
| shareButton.clearAnalyticsHistory(); | |
| ``` | |
| // Load the analytics adapters script first, then: | |
| const { ConsoleAdapter } = window.SocialShareAnalytics; | |
| const shareButton = new SocialShareButton({ | |
| container: "#share-button", | |
| analyticsOptions: { | |
| sampleRate: 0.6, // keep volume down in load tests | |
| dedupeWindow: 3000, // suppress duplicate clicks in short windows | |
| dedupeBy: ["eventName", "platform","url"], | |
| historyLimit: 150, | |
| enrichContext: true, | |
| includeUserAgent: false, | |
| }, | |
| analyticsPlugins: [new ConsoleAdapter()], | |
| onAnalytics: (payload) => { | |
| // custom filtering + forwarding | |
| if (payload.eventName !== "social_share_popup_open") { | |
| sendTelemetry(payload); | |
| } | |
| }, | |
| }); | |
| console.log(shareButton.getAnalyticsHistory()); | |
| shareButton.clearAnalyticsHistory(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 123 - 145, The README example references
ConsoleAdapter but doesn't show how to obtain or import it, causing a
ReferenceError; update the snippet to either (a) show the import/registration
for ConsoleAdapter (e.g., an import or adapter factory call) before new
SocialShareButton(...) so ConsoleAdapter is defined, or (b) add a short note and
link to the analytics integration docs explaining how to load ConsoleAdapter;
reference the symbols ConsoleAdapter, SocialShareButton, and analyticsPlugins so
readers can see where to add the adapter setup.
Comment on lines
+249
to
+267
| class GoogleTagManagerAdapter extends SocialShareAnalyticsPlugin { | ||
| track(payload) { | ||
| if (typeof window === "undefined" || !Array.isArray(window.dataLayer)) { | ||
| return; | ||
| } | ||
| window.dataLayer.push({ | ||
| event: payload.eventName, | ||
| source: payload.source, | ||
| interactionType: payload.interactionType, | ||
| platform: payload.platform, | ||
| url: payload.url, | ||
| title: payload.title, | ||
| componentId: payload.componentId, | ||
| timestamp: payload.timestamp, | ||
| ...(payload.errorMessage ? { errorMessage: payload.errorMessage } : {}), | ||
| ...(payload.context ? { context: payload.context } : {}), | ||
| }); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add inline comment for track() method.
Per coding guidelines for src/**/*.{js,jsx}, newly added methods should have brief inline comments explaining what they do, especially for non-obvious logic like the conditional spread operators.
📝 Suggested comment
class GoogleTagManagerAdapter extends SocialShareAnalyticsPlugin {
+ // Pushes analytics event to GTM dataLayer; includes errorMessage/context only when present
track(payload) {
if (typeof window === "undefined" || !Array.isArray(window.dataLayer)) {
return;
}Based on learnings: "Flag any newly added or modified function that lacks a descriptive comment explaining what it does."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/social-share-analytics.js` around lines 249 - 267, Add a brief inline
comment above the GoogleTagManagerAdapter.track method describing its purpose
and behavior: state that it guards against server-side execution (checks typeof
window), ensures window.dataLayer is an array, and then pushes a standardized
analytics payload to the GTM dataLayer, including optional conditional spreads
for errorMessage and context; update the comment to mention why the conditional
spread operators are used to avoid adding undefined fields.
Comment on lines
+695
to
+701
| /** | ||
| * Generates a short unique event ID for each emitted analytics event. | ||
| * @returns {string} | ||
| */ | ||
| _generateEventId() { | ||
| return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}`; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding brief inline comment explaining the ID format.
Per coding guidelines, inline comments are preferred over JSDoc for this codebase. A short comment explaining the format would aid maintainability:
📝 Suggested enhancement
_generateEventId() {
+ // Format: {base36-timestamp}-{random-8-chars} for lightweight client-side uniqueness
return `${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 10)}`;
}As per coding guidelines: "Add comments for logical blocks explaining what they do."
🤖 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 695 - 701, Add a brief inline
comment for the _generateEventId() function that explains the event ID format
and purpose: note that it returns a base36 timestamp followed by a hyphen and an
8-character base36 random string (timestamp for ordering + random suffix for
uniqueness), and place the comment immediately above the function declaration
(not as JSDoc) so readers quickly understand the ID structure used for emitted
analytics events.
Comment on lines
+784
to
+792
| // Analytics sample rate and filtering | ||
| const sampleRate = Number(analyticsOptions.sampleRate || 1); | ||
| if (sampleRate <= 0 || sampleRate > 0 && sampleRate < 1 && Math.random() > sampleRate) { | ||
| return; | ||
| } | ||
|
|
||
| if (typeof analyticsOptions.filter === "function" && !analyticsOptions.filter(payload)) { | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add inline comment explaining sampling logic.
The conditional on line 786 is correct but dense. An inline comment would clarify the edge cases (especially that values > 1 are treated as 100%):
📝 Suggested enhancement
// Analytics sample rate and filtering
+ // sampleRate <= 0: disabled; 0 < sampleRate < 1: probabilistic; sampleRate >= 1: 100% pass-through
const sampleRate = Number(analyticsOptions.sampleRate || 1);
if (sampleRate <= 0 || sampleRate > 0 && sampleRate < 1 && Math.random() > sampleRate) {
return;
}As per coding guidelines: "Add comments for edge cases and non-obvious logic."
📝 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
| // Analytics sample rate and filtering | |
| const sampleRate = Number(analyticsOptions.sampleRate || 1); | |
| if (sampleRate <= 0 || sampleRate > 0 && sampleRate < 1 && Math.random() > sampleRate) { | |
| return; | |
| } | |
| if (typeof analyticsOptions.filter === "function" && !analyticsOptions.filter(payload)) { | |
| return; | |
| } | |
| // Analytics sample rate and filtering | |
| // sampleRate <= 0: disabled; 0 < sampleRate < 1: probabilistic; sampleRate >= 1: 100% pass-through | |
| const sampleRate = Number(analyticsOptions.sampleRate || 1); | |
| if (sampleRate <= 0 || sampleRate > 0 && sampleRate < 1 && Math.random() > sampleRate) { | |
| return; | |
| } | |
| if (typeof analyticsOptions.filter === "function" && !analyticsOptions.filter(payload)) { | |
| return; | |
| } |
🤖 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 784 - 792, Add a brief inline
comment above the sampling conditional that explains the sampling behavior in
plain terms: compute sampleRate from analyticsOptions.sampleRate (defaults to
1), treat any value > 1 as 100% (no sampling), treat values between 0 and 1 as
probabilistic sampling using Math.random(), and short-circuit when sampleRate <=
0; also note that the subsequent analyticsOptions.filter(payload) is an
independent filter step. Place this comment near the sampleRate
declaration/conditional so readers of analyticsOptions, sampleRate, and the
payload/filter logic immediately understand the edge cases.
Comment on lines
+803
to
+811
| if (typeof analyticsOptions.dedupeBy === "function") { | ||
| sameEvent = analyticsOptions.dedupeBy(this._lastAnalyticsEvent, payload); | ||
| } else if (Array.isArray(analyticsOptions.dedupeBy)) { | ||
| sameEvent = analyticsOptions.dedupeBy.every((key) => | ||
| Object.prototype.hasOwnProperty.call(payload, key) | ||
| ? this._lastAnalyticsEvent[key] === payload[key] | ||
| : false | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Edge case: non-existent keys in dedupeBy array disable deduplication entirely.
If a developer specifies a key that doesn't exist in the payload (e.g., dedupeBy: ["eventName", "typoKey"]), the every() will always return false, effectively disabling deduplication silently.
Consider either logging a debug warning for invalid keys, or skipping non-existent keys:
📝 Alternative approach that skips missing keys
} else if (Array.isArray(analyticsOptions.dedupeBy)) {
- sameEvent = analyticsOptions.dedupeBy.every((key) =>
- Object.prototype.hasOwnProperty.call(payload, key)
- ? this._lastAnalyticsEvent[key] === payload[key]
- : false
- );
+ // Only compare keys that exist in both payloads
+ const validKeys = analyticsOptions.dedupeBy.filter(
+ (key) => Object.prototype.hasOwnProperty.call(payload, key)
+ );
+ sameEvent = validKeys.length > 0 && validKeys.every(
+ (key) => this._lastAnalyticsEvent[key] === payload[key]
+ );
}🤖 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 803 - 811, The current Array branch
for analyticsOptions.dedupeBy uses every() which returns false if any key is
missing, silently disabling dedupe; update the logic in
src/social-share-button.js where sameEvent is set for analyticsOptions.dedupeBy
so that it first filters the dedupe keys to those actually present on both
payload and this._lastAnalyticsEvent (or alternatively log a debug/warning for
missing keys) and then compare only those existing keys for equality; ensure you
still handle the case where no valid keys remain (e.g., treat as not-duplicate
or log) so deduplication behavior is explicit.
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.
Overview
This pull request improves the overall user interface and accessibility of the Social Share Button component. The goal is to enhance usability, ensure better interaction across devices, and align the component with modern UI/UX standards.
Changes Made
Why This Matters
These improvements make the component more user-friendly and inclusive. Enhanced accessibility ensures that users relying on assistive technologies can interact with the component effectively, while UI refinements improve overall engagement and usability.
How to Test
Clone the repository and run the project locally
Navigate to the Social Share Button component
Verify:
Future Improvements
Additional Notes
I would appreciate any feedback and am happy to make further improvements if required.
Summary by CodeRabbit
Release Notes
New Features
Documentation