-
Notifications
You must be signed in to change notification settings - Fork 117
Use Toasts for Success and Error Alerts #2675
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.
Great to see this, looking much better than before.
Updated in 2610ca0 with some better types and hook logic |
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.
Seems fine, I'd be inclined to stringify the errors on the way in but we might want to keep them intact in case we need them later.
function formatMessage(message: string | Error) { | ||
let formattedMessage = String(message); | ||
if (formattedMessage.startsWith("Error: ")) { | ||
formattedMessage = formattedMessage.replace("Error: ", ""); | ||
} | ||
|
||
return formattedMessage; | ||
} |
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.
Nitpick. This could be:
const formatMessage = (message: string | Error) => String(message).replace("Error: ", "");
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.
The startsWith
check seemed important, as I didn't want to mess with There was an Error: you broke it
(unlikely with that capital E in there, but you never know... it's an error).
This PR swaps our rather aggressive ErrorAlert and SuccessAlerts for Toasts! These toasts are 'pinned' to the UI (
position:fixed
) so you see them no matter where you are in the UI. Like before, they fade away after 4 seconds.This PR collapsed the APIs of
SucessHandler
andErrorHandler
to be the same, accepting the "message" to show to the user. This PR does not modify any of the content of these messages, except for removing a startingError:
on error messagesFor Reference, here's what things used to look like:
Checklists
Development
Impact
Please explain any security, performance, migration, or other impacts if relevant:
Code review