-
Notifications
You must be signed in to change notification settings - Fork 257
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
App strings reworked #1106
App strings reworked #1106
Conversation
app/src/main/res/values/strings.xml
Outdated
<string name="notification_backgroundOperations">Background operations</string> | ||
<string name="notification_error">Error</string> | ||
<string name="notification_incorrectCredentials">Incorrect credentials</string> | ||
<string name="notification_incorrectCredentials">Wrong username or password</string> |
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.
Anything else possible here?
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.
IIRC, incorrect client ID
and client secret
may cause that too. Maybe also things that cause HTTP 401 (like additional HTTP Basic auth), can't remember at the moment.
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.
If that happens to also be a thing to keep in mind, it can be added if it turns out to be a problem.
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.
What about "Wrong login data" or "Wrong credentials"?
Thank you for your work and sorry for the delayed in review. |
Forgot to mention: most of these comments are subjective and I'd like to see somebody else to weigh in on this. |
As far as i can see the changes requested by @tcitworld are resolved. It would be nice if you can approve the pull request. |
Hello @tcitworld and @di72nn , your both approvals on this PR are needed to get this merged. Otherwise merging is blocked |
Squashed, rebased, improved (in my opinion) some stuff. I guess I approve the PR in its current state, but I can't bring myself to re-read it again at the moment. |
@di72nn Good to go by me. I think it is more productive to give it another go sometime in the future, rather than getting too far down into diminishing returns :) |
+1
sounds good to me. when @tcitworld has another look over this issue, it can be merged. finally :) |
Needs a rebase |
No description provided.