-
Notifications
You must be signed in to change notification settings - Fork 15
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: show error UI when plugin has empty rows [DHIS2-16793] #3131
base: master
Are you sure you want to change the base?
fix: show error UI when plugin has empty rows [DHIS2-16793] #3131
Conversation
🚀 Deployed on https://pr-3131--dhis2-data-visualizer.netlify.app |
|
||
throw new EmptyResponseError() | ||
} | ||
ensureAnalyticsResponsesContainData( |
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 see that this wasn't previously in a try/catch either. Does this ever throw? What does the user experience in this case?
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 worked on this quite a while ago, but I am pretty sure it is like this (quite complicated):
onResponsesReceived
is defined inVisualization
and callsensureAnalyticsResponsesContainData
. It is passed as a prop toPluginWrapper
.- It is and then called by the
PluginWrapper
when a response comes in: it is called bydoFetchData
which in turn is called indoFetchAll
. doFetchAll()
returns a Promise and the catch method of this promise is called (doFetchAll().catch(..)
). In the catch calbackonError()
is being called with the error. So ifensureAnalyticsResponsesContainData()
throws an error this will be the parameter passed toonError(error)
onError
itself is another prop called by thePluginWrapper
but defined in theVisualization
. It ends up callingthis.props.setLoadError(error)
and this will ensure theStartScreen
is rendered with the appropriate message.
db2f7ce
to
4bdc978
Compare
Implements DHIS2-16793
Key features
Description
The key feature needs no further explanation but there are some things to take uinto consideration about the implementation:
PivotTable
component, and this is indeed where the error is thrown. However, this is really going wrong at an earlier stage: the filter condition has caused the rows to be empty, and really the plugin shouldn't have rendered thePivotTable
in the first place, but some sort of info/error screen instead. So this is what I have addressed, and I have ignored the bug in thePivotTable
. I am not sure we need to address this one.ErrorBoundary
component. I tried to implement things using one, but struggled to get this to work: somehow theonComponentDidCatch
hook never got triggered. When I realised that a solution without anErrorBoundary
was very simple (less code and no complexity), more in line with the implementation inStartScreen
, and worked at the first attempt, I decided to stick with that.TODO
Screenshots