-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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(feedback): Allow passing tags
field to any feedback config param
#12197
Conversation
size-limit report 📦
|
@@ -6,6 +6,8 @@ import type { | |||
FeedbackScreenshotIntegration, | |||
Integration, | |||
IntegrationFn, | |||
OptionalFeedbackConfiguration, |
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.
This feels very specific to be exported from types 🤔 I don't see us using this anywhere else? We should really only put stuff in the types package that absolutely has to be there, as this becomes public API for other stuff too etc!
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.
Justification for exportingOptionalFeedbackConfiguration
is because exported because it's part of the public api: it's the options for the integration constructor.
The other type OverrideFeedbackConfiguration
is also part of the public api, it's used by all the public functions like attachTo
& createWidget
and so on. It's slightly different from the one above with regards to theme stuff that can't currently be overridden.
So that was the reason, lmk what you think. I use the types changes like https://github.com/getsentry/pokemon-market/pull/7/files. There's a similar bit of code in getsentry that is untyped right now, but could/should be typed.
I touched the both in the pr to highlight the source
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.
Can we give it a better? OptionalFeedbackConfiguration
sounds like it should be an internal type rather than public facing
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.
gimme a suggestion and we'll do it!
…fic guard before setTags
@@ -59,6 +59,7 @@ sentryTest('should capture feedback', async ({ getLocalTestUrl, page }) => { | |||
}, | |||
}, | |||
level: 'info', | |||
tags: {}, |
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.
Can we add some tests showing that the tags are flowing through?
Depends on getsentry/sentry-javascript#12197 Follows instructions from getsentry/sentry-docs#10137 --------- Co-authored-by: Catherine Lee <55311782+c298lee@users.noreply.github.com>
We were missing the ability to set tags within feedback before, this corrects it.
See getsentry/sentry-docs#10137 for the docs update.
Now a developer can set
tags: {...}
anytime an options param is passed into feedback. This includes:feedbackIntegration({tags: {hello: 'world'}})
feedback.attachTo(element, {tags: {hello: 'world'}})
feedback.createWidget({tags: {hello: 'world'}})
feedback.createForm({tags: {hello: 'world'}})
Users can also pass tags to
sendFeedback()
which is slightly nicer than having to set them on scope first.Sentry.sendFeedback({tags: {hello: 'world'}})
captureFeedback()
is unaffected, keeping it closer in style to the othercapture*()
methods.I also took the chance to re-use related types in more places. Checkout the first 2 commits on the branch to see those changes individually.
Fixes getsentry/sentry#71254