Update error in AppView to use ErrorAlert. #4199
Conversation
Signed-off-by: Michael Nelson <minelson@vmware.com>
dashboard/src/actions/apps.ts
Outdated
| dispatch(errorApp(new FetchError(e.message))); | ||
| dispatch( | ||
| errorApp( | ||
| e instanceof Error |
There was a problem hiding this comment.
I've just followed the pattern which you'd done elsewhere Rafa, but wondering when e would not be an instance of Error? Guessing you saw something else, but it still had an e.message, so confused?
There was a problem hiding this comment.
Don't really remember, probably due to the any in } catch (e: any) {. But given that it is a catch, I'm with you, can't think of a situation where e would not be an Error.
There was a problem hiding this comment.
Great, I'll update both this one and the one you'd added before landing.
| it("should render string messages", () => { | ||
| const wrapper = mount(<ErrorAlert>foo</ErrorAlert>); | ||
| expect(wrapper.text()).toContain("foo"); | ||
| }); |
There was a problem hiding this comment.
See comment below on the interface for more info why I thought to remove this.
| @@ -8,7 +8,8 @@ import { CustomError } from "shared/types"; | |||
| import "./ErrorAlert.css"; | |||
|
|
|||
| export interface IErrorAlert { | |||
| children: CustomError | Error | string; | |||
| error: CustomError | Error; | |||
There was a problem hiding this comment.
When changing to an error prop, given that this is an ErrorAlert component (ie. something that should be used when you have an error), it no longer seemed to make sense to support a plain string here since (a) why use an ErrorAlert in that case, and (b) even if you wanted to, you could put the string as the child.
There was a problem hiding this comment.
There could be use cases where we need to show a plain text message as an error on UI (red, etc.), but agree with you that it can be done with (b).
|
Awesome, thanks! |
dashboard/src/actions/apps.ts
Outdated
| e instanceof Error | ||
| ? new FetchError("Unable to get installed package", [e]) | ||
| : new FetchError(`Unable to get installed package: ${e.message}`), |
There was a problem hiding this comment.
| e instanceof Error | |
| ? new FetchError("Unable to get installed package", [e]) | |
| : new FetchError(`Unable to get installed package: ${e.message}`), | |
| new FetchError("Unable to get installed package", [e]) |
Signed-off-by: Michael Nelson <minelson@vmware.com>
Signed-off-by: Michael Nelson minelson@vmware.com
Description of the change
Initially was just to update the error displayed in the AppView to use the
ErrorAlertcomponent, but theErrorAlertcomponent didn't currently support child elements like the old one did, which the AppView uses. So after chatting with Rafa, I've updated that component to take the error as a prop allowing the children to be normal react nodes.Benefits
Before:
After:
Clicking on the "Try again" does what it always did, re-issuing the request and rendering the view when it succeeds.
Possible drawbacks
Applicable issues
ErrorAlertcomponent to accept other children for rendering (in Appview) #4190Additional information