-
Notifications
You must be signed in to change notification settings - Fork 0
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
feat: Add sharing support #264
Conversation
There's some weird issue here with linting. Linting works locally but on the actions it fails. |
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.
The main thing we are missing for this to be working properly is having a
superfeed/:id
route that we can actually share. Otherwise, what are we sharing right now?
EDIT: Discussed this with @faraggi today: We are gonna go for the simples solution first, which is just to share the url.
We'll then see if we can work on a BE proxy so that we can keep users in the app.
describe("shareUtils", () => { | ||
describe("canShare", () => { | ||
it("should return true if the Web Share API is supported", () => { | ||
expect(canShare()).toBe(true); |
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.
weird that this is true during a test, which is run on node 🤔
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.
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.
Yes, sharing ideally doesn't work on desktop. We're making use of the user's device native capabilities to handle sharing. Note: I did research into the possibility of enabling sharing for desktop and there's a package we could use. However, since the superfeed is mobile-only, I see no reason to implement this unless you guys think otherwise. cc: @v-almonacid @faraggi |
I'll deploy this branch to epsilon then and test on mobile. |
Let's focus on mobile only 👍 |
Works on mobile. Please resolve conflicts. |
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.
Please resolve conflicts.
No description provided.