- Notifications
You must be signed in to change notification settings - Fork914
refactor(site): Refactor alerts#7587
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| - name: "@mui/material/Alert" | ||
| message: | ||
| "You should use the Alert component provided on | ||
| components/Alert/Alert" |
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.
Nice!
Feels like the icon on the "info" severity alert doesn't have enough contrast: |
Kira-Pilot commentedMay 17, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I think I was under the mistaken impression that we were going to be using the snackbar from this PR as the new alert widget going forward. Is that just for version updates? When do you think we should use an alert vs a toast vs a snackbar? Asking so I can get it right in my upcoming PR :) |
| import Button from "@mui/material/Button" | ||
| export interface AlertProps extends PropsWithChildren { | ||
| severity: MuiAlertProps["severity"] |
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.
IfErrorAlert is going to be a separate component, should we omit the severityerror from this prop type? At first glance, it's hard to tell if I should be usingErrorAlert for errors, orAlert withseverity="error" or both in different ways.
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.
Alert is the generic one with extra things and it should still be usable for errors if you need some customization. The advantage of using theErrorAlert component is, you can pass the error and get the message and detail displayed without having to do it yourself.
| severity="error" | ||
| text="No authentication methods configured!" | ||
| /> | ||
| <Alert severity="error">No authentication methods configured!</Alert> |
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.
Related to my comment above, what is the difference between these two ways of displaying errors?
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 answered in the previous comment.
| setOpen(false) | ||
| onDismiss && onDismiss() | ||
| }} | ||
| data-testid="dismiss-banner-btn" |
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.
Should we stick anaria-label on 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.
Since it has a text, I think we don't need it 🤔 but I'm not sure if I understood your point
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.
No, you're right! I had to google it but I think since we have the text we can forget the label.
Uh oh!
There was an error while loading.Please reload this page.
This is a REALLY good question and I'm not sure if I have a good answer for that, but I think snack bars are good for displaying errors or warnings that need to be displayed after a certain action as feedback like an update or delete error after a user clicked on a button. Alerts are good for static warnings where the users didn't trigger any action like deprecation notices and errors during initial loading but any of these are "written in stone" and we may find situations where display alerts for actions feedback or display snack bars for static warnings can be better, the PR that you mentioned is one of these cases. We didn't have a good place to place the "global alert" so it was just better and easy to make it a snack bar. |
This seems like a good distinction to me! |
Motivation:
Our alerts were inconsistent when displaying messages with details or actions, looking a bit ugly and unprofessional.
Changes:
AlertBannertoAlert.Alertis our custom component with a few standard features we usually need when displaying errors and warnings.ErrorAlert. It extends theAlertcomponent and uses thegetErrorMessageandgetErrorDetailto mount the text that will be displayedPreview:
Before:
After:
Before:
After:
