- Notifications
You must be signed in to change notification settings - Fork928
feat: New static error summary component#3107
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
Uh oh!
There was an error while loading.Please reload this page.
| return ( | ||
| <Stack> | ||
| <Stack className={styles.root}> |
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.
Does this outermost element need to be aStack or can it just be a fragment, i.e.<></>?
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.
Adds thegap for theretry component. I am fine with keeping this as aStack.
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.
Refactored it now!
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, that looks great!
| const useStyles = makeStyles<Theme, StyleProps>((theme) => ({ | ||
| root: { | ||
| background: `${theme.palette.error.main}60`, | ||
| margin: `${theme.spacing(2)}px`, |
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.
Just a suggestion: this component might be a little more flexible if we leave off the margin and instead let the parent decide what the margin should be (it will probably vary). If you feel like it, you could even pass in astyle prop so that the parent can pass through its own styles.
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 think we're going to use this component a lot and I'd like it to look decent without additional styling. Maybe we can give it margins we think are typical and then override when necessary.
Uh oh!
There was an error while loading.Please reload this page.
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 looks super!
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 finishing touches!
This PR repurposes the existing ErrorSummary component to create a static component to display error messages with details from the backend.
We want to make the details section collapsible and the error summary optionally dismissible.
Subtasks
Fixes#3106
Screenshots
Dismissible
Collapsed details
With details