Various bug fixes #185
Various bug fixes #185
Conversation
nofurtherinformation
had a problem deploying
to
pr-185
GitHub Actions
Failure
nofurtherinformation
temporarily deployed
to
pr-185
GitHub Actions
Inactive
nofurtherinformation
temporarily deployed
to
pr-185
GitHub Actions
Inactive
nofurtherinformation
temporarily deployed
to
pr-185
GitHub Actions
Inactive
nofurtherinformation
temporarily deployed
to
pr-185
GitHub Actions
Inactive
There was a problem hiding this comment.
LGTM and everything was working nicely in https://districtr-v2-185-app.fly.dev/
| <Text as="p" mb={'4'}> | ||
| {!!errorNotification?.severity && <i>Severity: {errorNotification.severity}</i>} | ||
| </Text> |
There was a problem hiding this comment.
PP: Not sure we need the severity here.
Q: Do you know if Sentry will capture these errors? It would be good to still have them tracked there but not pressing to figure that out
There was a problem hiding this comment.
I'm using severity as a way to sort alert type - dialog, toast, silent log ?
There was a problem hiding this comment.
I'm not super familiar with sentry but I'm sure this can integrate with it
There was a problem hiding this comment.
Makes sense – just not sure it needs to be exposed to the user!
| @@ -16,6 +17,7 @@ export default function Map() { | |||
| <MapComponent /> | |||
| <MobileTopNav /> | |||
| <MapContextMenu /> | |||
| <ErrorNotification /> | |||
There was a problem hiding this comment.
Q: Should this be further up in the DOM in case other pages eventually need it?
There was a problem hiding this comment.
Possibly but I'm not sure if that will mess up SSR stuff
| import {AlertDialog, Button, Flex, Text} from '@radix-ui/themes'; | ||
| import * as Toast from '@radix-ui/react-toast'; | ||
|
|
||
| export const ErrorNotification = () => { |
There was a problem hiding this comment.
PP: Good for now but there's probably an argument to be made either way that this should be an error dialog.
There was a problem hiding this comment.
Severity 1 is a dialog!
| set({ | ||
| errorNotification: { | ||
| severity: 2, | ||
| message: `Breaking this geography failed. Please refresh this page and try again. If this error persists, please share the error code below the Districtr team.`, |
There was a problem hiding this comment.
Thank you!
This PR has a few bug fixes for previous flagged issues.
Description
Reviewers
Checklist
Screenshots (if applicable):