Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: make share sheet global #8563

Merged
merged 14 commits into from
Apr 26, 2023
Merged

refactor: make share sheet global #8563

merged 14 commits into from
Apr 26, 2023

Conversation

gkartalis
Copy link
Member

@gkartalis gkartalis commented Apr 24, 2023 •
edited
Loading

This PR resolves []

Description

  • unifies all share sheet implementations that we have
  • makes customsharesheet global
  • unifies analytics

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

  • make share sheet global - gkartalis

iOS user-facing changes

Android user-facing changes

Dev changes

Need help with something? Have a look at our docs, or get in touch with us.

@gkartalis gkartalis self-assigned this Apr 24, 2023
@gkartalis gkartalis changed the title refactor(): make share sheet global refactor(WIP): make share sheet global Apr 24, 2023
damassi
damassi previously approved these changes Apr 24, 2023
View reviewed changes
Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good 👍 Just a couple minor suggestions + one request (unifying provider + context into one file); otherwise :shipit:

gkartalis
gkartalis commented Apr 25, 2023
View reviewed changes
src/app/Components/ShareSheet/ShareSheet.tsx
const res = await Share.open(shareOptions)
trackEvent(share(tracks.iosShare(res.message, data!.internalID, data.slug)))
} catch (err) {
Sentry.captureMessage("HANDLE_SHARE_MORE_PRESS: " + err)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe attatch the screen name here? (see relay error middleware)

@gkartalis gkartalis changed the title refactor(WIP): make share sheet global refactor: make share sheet global Apr 25, 2023
@gkartalis gkartalis marked this pull request as ready for review April 25, 2023 18:12
damassi
damassi previously approved these changes Apr 25, 2023
View reviewed changes
Copy link
Member

@damassi damassi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, thanks for working through those changes 👍

@damassi damassi added Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green and removed Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green labels Apr 25, 2023
@damassi
Copy link
Member

damassi commented Apr 25, 2023

(Merge when ready)

gkartalis and others added 4 commits April 26, 2023 12:10
@gkartalis
fix: remove unsafe weburl
857a789
@damassi @gkartalis
Update src/app/Components/Artist/ArtistHeaderFloatingButtons.tsx
8c8c159 
Co-authored-by: George Kartalis <g.kartalis7@gmail.com>
@damassi @gkartalis
Update src/app/Scenes/Artwork/Components/ArtworkHeader.tsx
c75686d 
Co-authored-by: George Kartalis <g.kartalis7@gmail.com>
@damassi @gkartalis
Update src/app/Scenes/Sale/Components/SaleHeader.tsx
e52cd6f 
Co-authored-by: George Kartalis <g.kartalis7@gmail.com>
@gkartalis gkartalis force-pushed the gkartalis/global-share-sheet branch from 6a4bd4f to e52cd6f Compare April 26, 2023 10:10
@gkartalis gkartalis added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Apr 26, 2023
@artsy-peril artsy-peril bot merged commit 7c7d538 into main Apr 26, 2023
@artsy-peril artsy-peril bot deleted the gkartalis/global-share-sheet branch April 26, 2023 11:23
@gkartalis gkartalis mentioned this pull request Apr 27, 2023
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damassi damassi damassi left review comments

Assignees

@gkartalis gkartalis

Labels
Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gkartalis @damassi