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.

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(user feedback): initial API #4284

Merged
merged 39 commits into from
Sep 20, 2024
Merged

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Aug 15, 2024

An initial draft of the API we'll offer to customers for the new User Feedback widget and managed form, for #4269, including an example of how it would look to configure it, in iOS-Swift.

Docs PR: getsentry/sentry-docs#11191

#skip-changelog

@armcknight armcknight changed the title initial api outline feat(user feedback): initial api outline Aug 15, 2024
@armcknight armcknight changed the title feat(user feedback): initial api outline feat(user feedback): initial API Aug 15, 2024
Copy link

github-actions bot commented Aug 15, 2024

Messages
📖 Do not forget to update Sentry-docs with your feature once the pull request gets approved.

Generated by 🚫 dangerJS against ebb7c99

Copy link

codecov bot commented Aug 15, 2024

Codecov Report

Attention: Patch coverage is 0% with 86 lines in your changes missing coverage. Please review.

Project coverage is 91.556%. Comparing base (c45c91c) to head (ebb7c99).
Report is 9 commits behind head on main.

Files with missing lines Patch % Lines
...uration/SentryUserFeedbackThemeConfiguration.swift 0.000% 30 Missing ⚠️
...s/UserFeedback/SentryUserFeedbackIntegration.swift 0.000% 28 Missing ⚠️
...guration/SentryUserFeedbackFormConfiguration.swift 0.000% 20 Missing ⚠️
...ration/SentryUserFeedbackWidgetConfiguration.swift 0.000% 5 Missing ⚠️
...onfiguration/SentryUserFeedbackConfiguration.swift 0.000% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #4284       +/-   ##
=============================================
- Coverage   91.697%   91.556%   -0.142%     
=============================================
  Files          622       627        +5     
  Lines        50502     50616      +114     
  Branches     18251     18347       +96     
=============================================
+ Hits         46309     46342       +33     
- Misses        4102      4182       +80     
- Partials        91        92        +1     
Files with missing lines Coverage Δ
Sources/Sentry/SentrySDK.m 88.950% <ø> (ø)
...onfiguration/SentryUserFeedbackConfiguration.swift 0.000% <0.000%> (ø)
...ration/SentryUserFeedbackWidgetConfiguration.swift 0.000% <0.000%> (ø)
...guration/SentryUserFeedbackFormConfiguration.swift 0.000% <0.000%> (ø)
...s/UserFeedback/SentryUserFeedbackIntegration.swift 0.000% <0.000%> (ø)
...uration/SentryUserFeedbackThemeConfiguration.swift 0.000% <0.000%> (ø)

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c45c91c...ebb7c99. Read the comment docs.

Copy link
Contributor

@brustolin brustolin 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, some comments.

Please, move the Swift files to "Swift/Integrations/FeedbackUser", also in the project organization.

Sources/Swift/SentryUserFeedbackConfiguration.swift Outdated Show resolved Hide resolved
Sources/Swift/SentryUserFeedbackConfiguration.swift Outdated Show resolved Hide resolved
Sources/Swift/SentryUserFeedbackFormConfiguration.swift Outdated Show resolved Hide resolved
Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

Maybe my comments dont' make sense if we're consider "user feedback" as the behavior of popping the modal on shake and all the modal settings. But since we've had an API (captureUserFeedback) for many years, I find that it might be confusing now with the new capabilities.

I wonder if we can find a better way to differentiate between the APIs to capture feedback (new captureFeedback since we're deprecating captureUserFeedback`) and any automatic behavior like poping up a modal on crash or on shake.

Samples/iOS-Swift/iOS-Swift/AppDelegate.swift Show resolved Hide resolved
Sources/Sentry/Public/SentryOptions.h Outdated Show resolved Hide resolved
@bruno-garcia
Copy link
Member

Also adding @c298lee and @billyvg from the Replay team (they are familiar with the Feedback SDK in JS)

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

This looks great, the biggest questions are how do we name the root config, do we remove user from the naming, what is the widget exactly and what is a form. We can also discuss this on a call cause these discussions could take forever on GH. The rest LGTM.

@armcknight armcknight force-pushed the armcknight/feat(user-feedback)/api branch from cebfd4d to 2bef25e Compare September 18, 2024 22:40
@armcknight armcknight force-pushed the armcknight/feat(user-feedback)/api branch from 92ce272 to 131c10b Compare September 18, 2024 22:45
@armcknight
Copy link
Member Author

armcknight commented Sep 18, 2024

automatic behavior like poping up a modal on crash or on shake

@bruno-garcia I do have the shake trigger in here already, but popping a dialog on crash detection is a good one too 👍🏻 Though I'd echo @philipphofmann 's comment to keep scope narrow for v1 and follow on with that.

Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

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

The user-facing API, LGTM.

@armcknight
Copy link
Member Author

Pushed a commit to remove this from public access while we merge intermediate PRs

@armcknight armcknight merged commit f0be869 into main Sep 20, 2024
60 of 65 checks passed
@armcknight armcknight deleted the armcknight/feat(user-feedback)/api branch September 20, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants