-
Notifications
You must be signed in to change notification settings - Fork 155
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: add report problem functionality #1359
feat: add report problem functionality #1359
Conversation
Signed-off-by: Bryce McMath <bryce.j.mcmath@gmail.com>
Signed-off-by: Bryce McMath <bryce.j.mcmath@gmail.com>
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.
Pre-review to clarify a couple things
@@ -20,30 +21,40 @@ export enum InfoBoxType { | |||
Error, | |||
} | |||
|
|||
interface BifoldErrorProps { |
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 must have been a copy paste mistake from way back. It's not exported, so I thought it safe to rename correctly.
useEffect(() => { | ||
const errorAddedHandle = DeviceEventEmitter.addListener(EventTypes.ERROR_ADDED, (err: BifoldError) => { | ||
if (err.title && err.message) { | ||
setError(err) | ||
setReported(false) |
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.
Every time a new error occurs, re-enable the report functionality.
@@ -50,7 +50,7 @@ const PINExplainer: React.FC<PINExplainerProps> = ({ continueCreatePIN }) => { | |||
<Trans | |||
i18nKey="PINCreate.Explainer.PINReminder" | |||
components={{ | |||
b: <Text style={TextTheme.labelTitle} />, | |||
b: <Text style={TextTheme.bold} />, |
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 is to use the correct font size so that the bolded text within the b
isn't slightly smaller than the rest of the sentence.
_log: any | ||
_config = { |
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.
private
fields cannot be overridden in extended classes, that's why these are now just prefixed with an underscore
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.
How do they get overridden? Do you need a setter or getter?
class Person {
private _name: string;
constructor(name: string) {
this._name = name;
}
get name(): string {
return this._name;
}
set name(newName: string) {
if (newName.length < 3) {
throw new Error("Name must be at least 4 characters long.");
}
this._name = newName;
}
}
"@credo-ts/core": "0.5.11", | ||
"@typescript-eslint/parser": "^6.6.0", | ||
"axios": "^1.4.0", | ||
"buffer": "^6.0.3", | ||
"eslint": "^8.48.0", | ||
"eslint-import-resolver-typescript": "^2.5.0", | ||
"react": "18.2.0", | ||
"react-native": "0.72.5", | ||
"react-native-logs": "^5.1.0", |
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.
Moved these to regular dependencies as they are used in the built package
Signed-off-by: Bryce McMath <bryce.j.mcmath@gmail.com>
Signed-off-by: Bryce McMath <bryce.j.mcmath@gmail.com>
secondaryCallToActionPressed={reportProblemAction && error ? () => reportProblemAction(error) : undefined} | ||
secondaryCallToActionTitle={reported ? t('Error.Reported') : t('Error.ReportThisProblem')} | ||
secondaryCallToActionDisabled={reported} | ||
secondaryCallToActionIcon={reported ? <Icon style={{ marginRight: 8 }} name={'check-circle'} size={18} color={ColorPallet.semantic.success} /> : undefined} |
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.
it would be nice to see this defined in a function and just pass the reported variable into it. I think it would make it easier to read
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.
done
Signed-off-by: Bryce McMath <bryce.j.mcmath@gmail.com>
Signed-off-by: Bryce McMath <bryce.j.mcmath@gmail.com>
Signed-off-by: Bryce McMath <bryce.j.mcmath@gmail.com>
|
Summary of Changes
This PR adds "Report this problem" functionality to the remote logging package and Bifold if you choose to enable it. I synthesized our use of
BaseLogger
andConsoleLogger
into oneBifoldLogger
that is used across the board. I also added react-native-logs as a direct dep of core, since it was already being used there. It also adds the option of including the app version and build number in the InfoBox component, which I've only made use of on the ErrorModal.Screenshots, videos, or gifs
Breaking change guide
TOKENS.UTILS_LOGGER
it will now be of typeBifoldLogger
now.reportProblemAction
callback, it is now just anenableReport
booleanRelated Issues
N/A
Pull Request Checklist
Tick all boxes below to demonstrate that you have completed the respective task. If the item does not apply to your this PR check it anyway to make it apparent that there's nothing to do.
Signed-off-by
line (we use the DCO GitHub app to enforce this)If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!
Pro Tip 🤓