-
Notifications
You must be signed in to change notification settings - Fork 22
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
#2174: Replace unmaintained react-toast-notifications
(and use one style for all toasts)
#2836
Changes from 4 commits
d25e72a
ba8a251
5d15dfd
d86906f
fead813
fa9685a
26808c8
c0f6dcb
aea6a63
5435bd9
7c3b739
27b64dc
3907d04
1e420d8
fc8c8fa
8a54ce3
12196f8
4394530
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,10 +20,7 @@ import browser, { Menus, Tabs } from "webextension-polyfill"; | |
import { getErrorMessage, hasCancelRootCause } from "@/errors"; | ||
import reportError from "@/telemetry/reportError"; | ||
import { noop } from "lodash"; | ||
import { | ||
handleMenuAction, | ||
showNotification, | ||
} from "@/contentScript/messenger/api"; | ||
import { handleMenuAction, notify } from "@/contentScript/messenger/api"; | ||
import { ensureContentScript } from "@/background/util"; | ||
import { reportEvent } from "@/telemetry/events"; | ||
import { UUID, IExtension, ResolvedExtension } from "@/core"; | ||
|
@@ -95,21 +92,16 @@ async function dispatchMenu( | |
extensionId: info.menuItemId.slice(MENU_PREFIX.length) as UUID, | ||
args: info, | ||
}); | ||
void showNotification(target, { | ||
message: "Ran content menu item action", | ||
type: "success", | ||
}); | ||
notify.success(target, "Ran content menu item action"); | ||
} catch (error) { | ||
if (hasCancelRootCause(error)) { | ||
void showNotification(target, { | ||
message: "The action was cancelled", | ||
}); | ||
notify.info(target, "The action was cancelled"); | ||
} else { | ||
const message = `Error handling context menu action: ${getErrorMessage( | ||
error | ||
)}`; | ||
reportError(new Error(message)); | ||
void showNotification(target, { message, type: "error" }); | ||
reportError(error); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we want to report the original error here. The stack trace will point to this block anyway, but its origin will be better defined. Here it's called explicitly because the messaging API does not automatically serialize errors, especially deep inside other objects. |
||
notify.error(target, { message, report: false }); | ||
} | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
/* | ||
* Copyright (C) 2022 PixieBrix, Inc. | ||
* Copyright (C) 2022´ PixieBrix, Inc. | ||
* | ||
* This program is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU Affero General Public License as published by | ||
|
@@ -19,7 +19,7 @@ import { Effect } from "@/types"; | |
import { proxyService } from "@/background/messenger/api"; | ||
import { propertiesToSchema } from "@/validators/generic"; | ||
import { BlockArg } from "@/core"; | ||
import { showNotification } from "@/contentScript/notify"; | ||
import notify from "@/utils/notify"; | ||
|
||
export class AddOrganization extends Effect { | ||
// https://developers.pipedrive.com/docs/api/v1/#!/Organizations/post_organizations | ||
|
@@ -62,7 +62,7 @@ export class AddOrganization extends Effect { | |
}); | ||
|
||
if (data.items.length > 0) { | ||
showNotification({ message: `Organization already exists for ${name}` }); | ||
notify.info(`Organization already exists for ${name}`); | ||
return; | ||
} | ||
|
||
|
@@ -72,15 +72,9 @@ export class AddOrganization extends Effect { | |
method: "post", | ||
data: { name, owner_id }, | ||
}); | ||
showNotification({ | ||
message: `Added ${name} to Pipedrive`, | ||
type: "success", | ||
}); | ||
notify.success(`Added ${name} to Pipedrive`); | ||
} catch { | ||
showNotification({ | ||
message: `Error adding ${name} to Pipedrive`, | ||
type: "error", | ||
}); | ||
notify.error(`Error adding ${name} to Pipedrive`); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As @twschiller suggested in https://github.com/pixiebrix/pixiebrix-app/pull/1252, errors are automatically reported. This was not the case with the previous There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These should actually be logger statements and BusinessErrors. Will change these |
||
} | ||
} | ||
} | ||
|
@@ -140,7 +134,7 @@ export class AddPerson extends Effect { | |
}); | ||
|
||
if (data.items.length > 0) { | ||
showNotification({ message: `Person record already exists for ${name}` }); | ||
notify.info({ message: `Person record already exists for ${name}` }); | ||
return; | ||
} | ||
|
||
|
@@ -155,15 +149,9 @@ export class AddPerson extends Effect { | |
phone: phone ? [phone] : undefined, | ||
}, | ||
}); | ||
showNotification({ | ||
message: `Added ${name} to Pipedrive`, | ||
type: "success", | ||
}); | ||
notify.success(`Added ${name} to Pipedrive`); | ||
} catch { | ||
showNotification({ | ||
message: `Error adding ${name} to Pipedrive`, | ||
type: "error", | ||
}); | ||
notify.error(`Error adding ${name} to Pipedrive`); | ||
} | ||
} | ||
} |
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.
API improvement: Either call it with a plain string or provide a regular
Notification
object.Pros:
{ error }
wrapunknown
as the first parameter ofnotify.error
just to allownotify.error(new Error())
Cons:
message:
if you need to include theerror
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'm not sure we have the budget for the extra storage