-
Notifications
You must be signed in to change notification settings - Fork 147
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
Integ 231 improve error handling within analytics app component #2919
Integ 231 improve error handling within analytics app component #2919
Conversation
0b40b81
to
0bc8116
Compare
apps/google-analytics-4/frontend/src/components/main-app/ErrorDisplay/ErrorDisplay.tsx
Outdated
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/main-app/ErrorDisplay/ErrorDisplay.tsx
Outdated
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/common/HyperLink/HyperLink.tsx
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/main-app/constants/noteMessages.ts
Outdated
Show resolved
Hide resolved
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.
Looks great to me! Just added a couple small comments/questions!
apps/google-analytics-4/frontend/src/components/main-app/constants/noteMessages.ts
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/main-app/ChartHeader/ChartHeader.styles.ts
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/main-app/constants/noteMessages.ts
Outdated
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/main-app/ErrorDisplay/ErrorDisplay.tsx
Outdated
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/main-app/ChartContent/ChartContent.tsx
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/common/HyperLink/HyperLink.tsx
Show resolved
Hide resolved
...oogle-analytics-4/frontend/src/components/main-app/SlugWarningDisplay/SlugWarningDisplay.tsx
Outdated
Show resolved
Hide resolved
apps/google-analytics-4/frontend/src/components/main-app/ErrorDisplay/ErrorDisplay.tsx
Outdated
Show resolved
Hide resolved
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.
Everything looked good as far as the stuff you mentioned UI/Error Handling and overall great work on much needed fixes!
Two things however
- Storing the React component in state.
- I found two unhandled error cases (which produces the screenshots below)
a. One where you kill the lambda (ie no server/server error/500s)
b. Invalid service key
Now this PR only says to improve the error handling. So if this is just a foundation PR for handling Errors, I'm cool with you creating a ticket and handling those fixes separately as your next task (I'm also fine with you fixing them in this PR).
The second point will be a relatively annoying refactor for you but I think it'll prevent a lot of headaches down the road and is worth fixing in this PR.
apps/google-analytics-4/frontend/src/components/main-app/AnalyticsApp/AnalyticsApp.tsx
Outdated
Show resolved
Hide resolved
84bf27e
to
94d1dcc
Compare
Purpose
This PR accomplishes the following:
Approach
Error handling:
I added a component here called the
ErrorDisplay
component that is rendered in place of the Chart itself when an error is thrown. This component handles the anticipated two most common errors, providing an invalid argument somehow which is unlikely (invalidpropertyId
.), or a disabled Data api error. If the error does not belong to one of these error types, then the provided error message is displayed. I added a comment on this code to perhaps adjust the logic to display an actionable default error message if the error is not properly identified by the frontend, instead of the "mystery" error message.Example error message for invalid arguments:
Small UI fixes:
Note content not wrapping appropriately:
Before:
After:
Outline of Date range Select menu cut-off on right side:
Before:
After: