-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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): [1/4] add dotnet platforms to issues feedback onboarding #66561
Conversation
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.
coule casing nits otherwise LGTM!
), | ||
code: [ | ||
{ | ||
label: 'Cshtml', |
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.
label: 'Cshtml', | |
label: 'cshtml', |
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.
should probably change this in the docs too then👀 https://docs.sentry.io/platforms/dotnet/guides/aspnet/user-feedback/
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.
ah i think docs automatically capitalizes the label name actually :(
introduction: () => getCrashReportModalIntroduction(), | ||
install: (params: Params) => getCrashReportGenericInstallStep(params), |
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.
just noticed, nit for sure, but this type of thing can be written as:
introduction: () => getCrashReportModalIntroduction(), | |
install: (params: Params) => getCrashReportGenericInstallStep(params), | |
introduction: getCrashReportModalIntroduction, | |
install: getCrashReportGenericInstallStep, |
as long as the props are the same (same types obvs, but also same number of props).
ie (params: Params)
matches with (params)
on like 214.
Places to be careful of this are like [].forEach
and [].map
because they pass 3 params into the callback, so if your callback is expecting an optional 2nd param it'll get something unexpected! ie: [1, 2, 3].forEach(console.log)
is will print more than [1,2,3].forEach(num => console.log(num))
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.
i think for the sake of having to change 60 more files i won't apply this suggestion for these PRs but will definitely keep this in mind for future PRs🫡
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, def for other times, not now.
…boarding (#66599) this PR adds the following platforms: - 'elixir', - 'go', - 'go-echo', - 'go-fasthttp', - 'go-gin', - 'go-http', - 'go-iris', - 'go-martini', - 'go-negroni', all follow pretty much the same structure as #66561 relates to #66162 <img width="461" alt="SCR-20240308-imvr" src="https://github.com/getsentry/sentry/assets/56095982/e09a34f5-39c3-43f0-9fdc-38763c456dab">
This PR adds the following platforms to issues feedback onboarding:
It also modifies the
useSourcePackageRegistries
hook to includefiles
in the datatype. It was always there being returned from the endpoint; the datatype just didn't have the property. We need to use this information to display some of the strings in the first JS snippet.Relates to #66162