feat(app): implement a global error boundary component #285
Conversation
|
@jennydaman can you please have a look? |
| </Button> | ||
| </div> | ||
| <div className="illustration-container"> | ||
| <img src={Illustration} alt="Error illustration" className="illustration" /> |
There was a problem hiding this comment.
Where did this illustration come from? Just want to make sure reuse is permitted.
Or perhaps (in a future issue) we should loop back with @mairin and consider using "brainy" the mascot here (similar to the 404 error page).
| <h2>{this.state.error?.message}</h2> | ||
| <p>We are having an issue, please click on the button bellow to reload the page !</p> | ||
|
|
||
| <Button id="reload-error-btn" variant="primary" onClick={() => this.reload()}> |
There was a problem hiding this comment.
Currently, the reload function doesn't necessarily reload the page but navigates us back to /. Do you think it would be better to try reloading the page, or navigating "home"? In the latter case, we should instead say here "please click on the button below to go home" (or maybe go back)?
There was a problem hiding this comment.
Currently, the
reloadfunction doesn't necessarily reload the page but navigates us back to/. Do you think it would be better to try reloading the page, or navigating "home"? In the latter case, we should instead say here "please click on the button below to go home" (or maybe go back)?
It would be better to navigate "home". I got your comments
| }; | ||
|
|
||
| reload = () => { | ||
| window.location.href = '/'; |
There was a problem hiding this comment.
Pedantic: don't assume web app is served from /, i.e. this will not work if deployed on a subpath (by setting PUBLIC_URL or homepage).
| /> | ||
| <div className="container"> | ||
| <div className="error-message-container"> | ||
| <h1>400</h1> |
There was a problem hiding this comment.
Not necessarily a 400 error, could be a client-side error.
| <div className="error-message-container"> | ||
| <h1>400</h1> | ||
| <h2>{this.state.error?.message}</h2> | ||
| <p>We are having an issue, please click on the button bellow to reload the page !</p> |
There was a problem hiding this comment.
Please proofread.
Idea: we should provide a link to https://github.com/FNNDSC/ChRIS_store_ui/issues
| return ( | ||
| <> | ||
| <ErrorNotification | ||
| key={`notif-boundary`} |
There was a problem hiding this comment.
Does our eslint config allow for unnecessary string template? If so, I should change that.
| message={this.state.error?.message || 'Something happened!'} | ||
| position="top-right" | ||
| variant="danger" | ||
| closeable |
There was a problem hiding this comment.
Is Notification component to use here? If an uncaught exception happens in a child component, I don't think a closable notification would be suitable, we would probably want the entire view to be replaced by an error screen (stop the user from interacting with the program when its state becomes undefined/erroneous)
This Pull Request is associated with this issue #206
Implement an error boundary component to handle errors that can corrupt the internal state of React.
Display a fallback UI to the user