fix: Move "invalid graph" warning to MapFieldInput
#3689
Conversation
DafyddLlyr
force-pushed
the
dp/move-invalid-graph-error-to-MapInputField
branch
from
d31b940 to
a53711e
Compare
DafyddLlyr
changed the title
MapFieldInputMapFieldInput
|
Removed vultr server and associated DNS entries |
There was a problem hiding this comment.
Working exactly as expected for List, just one question below about how much visibility we actually need here if mostly intended for editor feedback !
I think you've also missed removing the existing ErrorSummaryContainer from Map & Label, therefore still falling back to component first rather than field-level throw here ??
| throw Error( | ||
| 'Edit this flow so that this component is positioned after "FindProperty"; an address is required for schemas that include a "map" field.', | ||
| { cause: "Invalid graph" }, | ||
| ); |
There was a problem hiding this comment.
nit: I'm not sure I'm sold on throwing an error here rather than rendering a component as before - do we want this noise coming through Airbrake?
Understand we can't easily render the whole component any longer from this shared position, but just wondering if there's a mechanism to exclude from Airbrake here etc?
There was a problem hiding this comment.
Tested on the pizza and I still get the correct error messages. Although the format of the error is different to the error with the Map and Label component. I see this as separate to the purpose of this PR so happy to approve.
Should these be the same?
- List Comp Error
- Map and Label Comp
DafyddLlyr
force-pushed
the
dp/move-invalid-graph-error-to-MapInputField
branch
from
edcb87b to
b39f801
Compare
|
@RODO94 @jessicamcinchak Thanks both for the feedback, please see recent commits which make the following changes -
|
There was a problem hiding this comment.
There's some basic tests here currently missing, I'll see if I can set this up in a follow up PR.
| function ErrorFallback(props: { error: Error }) { | ||
| logger.notify(props.error); | ||
| const ErrorFallback: React.FC<{ error: Error }> = ({ error }) => { | ||
| if (isGraphError(error)) return <GraphErrorComponent error={error} />; |
There was a problem hiding this comment.
This is a neat solution & I think we'll appreciate not having extra noise from these, thanks!
What?
Moves the "invalid graph" warning from a high level component, to the lower level
MapFieldInputWhy?
I noticed when working on 11aa28a that I also needed to handle this error in the
Pagecomponent. The actual component responsible for this dependency is theMapFieldInput, so it makes sense to do the error handling here once as opposed to remembering to do it in each parent which renders this component.