-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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): Make "required" text for input elements configurable #11153
Conversation
Thanks for the PR @soerface! We could potentially reduce it to two -- we definitely want the notice about ad blockers as that is something useful to the end user. Having specific error messages helps us debug too. I think we should have 3 configurable options for the different error messages: "client error", "server error", "unknown error" |
7078d4b
to
c319ea9
Compare
Thanks for your feedback @billyvg! I've brought the branch up to date with master, and added three configurable messages as requested:
The current version is difficult for me to test, since the version on master still throws How do you feel about the three extra parameters to |
c319ea9
to
b1895c3
Compare
@soerface Ah right, I was not considering what do you think about removing the error text changes for now and we can get the required text merged in first as that is more straight forward? |
b1895c3
to
323fa03
Compare
@billyvg sure! I've simplified the PR, merely making |
The configuration option was added in getsentry/sentry-javascript#11153.
I've also created a PR in the documentation repository. Regarding this:
Should we continue a discussion about that in the issue? I do think it is valuable to have it configurable. Because, as you already mentioned:
Therefore, as a site-owner, I also want to make sure that this message about ad-blockers can be read by my users in their native language. But maybe we also don't need a config option for this, if the SDK / "core" can handle translating user-facing error messages. |
The configuration option was added in getsentry/sentry-javascript#11153. Co-authored-by: vivianyentran <[email protected]>
Looks like it's failing a type check:
|
7cd0dcf
to
ac96a15
Compare
Co-authored-by: Billy Vong <[email protected]>
…le (#11287) Backport of #11153 From the OP (h/t @soerface): This introduces an option `isRequiredLabel`, and I planned to also add `errorMessageText`. However, this PR is branched off from a version that is a couple days old. Right now, @c298lee is currently working on getsentry/sentry#63749, which is causing major changes and the current version on master doesn't work yet. Therefore, I didn't yet implement `errorMessageText`. So consider this PR as a PoC, and either feel free to tag me when the screenshot changes are done – then I'll redo the changes based on the version that supports screenshots – or add the option on your own; however you prefer 🙂 --------- Co-authored-by: Soeren Wegener <[email protected]>
The configuration option was added in getsentry/sentry-javascript#11153. Co-authored-by: vivianyentran <[email protected]>
…etsentry#11152) (getsentry#11153) Fix getsentry#11152 This introduces an option `isRequiredLabel`, and I planned to also add `errorMessageText`. However, this PR is branched off from a version that is a couple days old. Right now, @c298lee is currently working on getsentry/sentry#63749, which is causing major changes and the current version on master doesn't work yet. Therefore, I didn't yet implement `errorMessageText`. So consider this PR as a PoC, and either feel free to tag me when the screenshot changes are done – then I'll redo the changes based on the version that supports screenshots – or add the option on your own; however you prefer 🙂 One open question: Until now, there was only the error message: > There was a problem submitting feedback, please wait and try again. Now, depending on the status code, we have three error messages: > - 'Unable to send Feedback. This is because of network issues, or because you are using an ad-blocker.' > - 'Unable to send Feedback. Invalid response from server.' > - 'Unable to send Feedback' Do you have a suggestion how we could make the message configurable, without introducing too many and redundant settings? Maybe we should go back to only one message? I'm not sure if an end-user cares about whether it is a network issue or a server error. --------- Co-authored-by: Billy Vong <[email protected]>
Fix #11152
This introduces an option
isRequiredLabel
, and I planned to also adderrorMessageText
.However, this PR is branched off from a version that is a couple days old. Right now, @c298lee is currently working on getsentry/sentry#63749, which is causing major changes and the current version on master doesn't work yet. Therefore, I didn't yet implement
errorMessageText
.So consider this PR as a PoC, and either feel free to tag me when the screenshot changes are done – then I'll redo the changes based on the version that supports screenshots – or add the option on your own; however you prefer 🙂
One open question:
Until now, there was only the error message:
Now, depending on the status code, we have three error messages:
Do you have a suggestion how we could make the message configurable, without introducing too many and redundant settings? Maybe we should go back to only one message? I'm not sure if an end-user cares about whether it is a network issue or a server error.