-
Notifications
You must be signed in to change notification settings - Fork 65
feat: add twitter support #36
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add twitter support #36
Conversation
WalkthroughAdds Twitter/X platform support: demo UI updated with Twitter references and placeholder, CSS hover style for Twitter added, new TypeScript twitterHandler implemented and exported, and Platform type extended to include 'twitter'. package.json eslint version changed. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
apps/demo/index.html (2)
83-83: Update placeholder to include Instagram.The placeholder mentions "YouTube/LinkedIn/Twitter" but omits Instagram, which is also supported (line 57-63).
🔎 Proposed fix
- placeholder="Paste a URL here (YouTube/LinkedIn/Twitter)" + placeholder="Paste a URL here (YouTube/LinkedIn/Instagram/Twitter)"
1-112: The PR description mentions unresolved permission/cancel behavior issues.The PR description reports two issues that are not addressed in this code:
- When a user clicks "cancel" on the permission prompt, the flow immediately opens the web link
- After one cancel, subsequent attempts always open the web link instead of prompting for the app
These issues relate to the app-opening logic (likely in main.ts, which is not included in this PR) rather than just adding Twitter support.
Would you like me to help investigate the permission handling logic? I can generate a script to examine the main.ts file and related code to understand the current implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
apps/demo/index.htmlapps/demo/src/style.csspackage.jsonpackages/core/src/platforms/index.tspackages/core/src/platforms/twitter.tspackages/core/src/types.ts
🧰 Additional context used
🧬 Code graph analysis (1)
packages/core/src/platforms/twitter.ts (2)
packages/core/src/platforms/index.ts (1)
twitterHandler(12-12)packages/core/src/types.ts (1)
DeepLinkHandler(10-13)
🔇 Additional comments (6)
packages/core/src/types.ts (1)
1-1: LGTM!The Platform type correctly includes 'twitter' and maintains consistency with the new twitterHandler implementation.
apps/demo/src/style.css (1)
111-113: LGTM!The Twitter link hover styling is consistent with existing platform link styles and uses Twitter's official brand color.
packages/core/src/platforms/index.ts (1)
5-5: LGTM!The twitterHandler import and export are correctly integrated with the existing platform handlers.
Also applies to: 11-12
apps/demo/index.html (1)
64-71: LGTM!The Twitter example link is correctly implemented with appropriate styling classes and data attributes, consistent with other platform examples.
packages/core/src/platforms/twitter.ts (1)
11-12: The iOS schemetwitter://user?screen_name=and Android packagecom.twitter.androidremain valid and unchanged after Twitter's rebrand to X. The Android app's package name remains com.twitter.android, and the iOS deep linking scheme continues to supporttwitter://user?screen_name=format. No updates are required for these identifiers.package.json (1)
15-15: No action needed - ESLint version is correctly pinned. ESLint 8.57.0 is fully supported by @typescript-eslint/eslint-plugin 8.50.0 (supports ^8.57.0 || ^9.0.0). This is an intentional choice, not a workaround for compatibility issues.
|
i have solved the problems you have stated with the changes i have made can you please review |
I added twitter support.
ISSUE: with demo that if a user accidently clicks cancel when it asks for the permission to open the app it directly opens the web link.
SUGGESTION: it should ask another question that the user wans to open in browser or don't want to open the app.
ISSUE: if the user cancels it once then the it always opens the web link not the app for the specific handler the user cancelled this nullifies the whole point of this app for the user if he/she clicks cancel by mistake
Summary by CodeRabbit
New Features
Style
Chores
✏️ Tip: You can customize this high-level summary in your review settings.