-
Notifications
You must be signed in to change notification settings - Fork 16
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
126 add error handling mechanism to the UI #518
base: develop
Are you sure you want to change the base?
Conversation
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.
Just a drive-by comment about brand name. 😄
flex-direction: column; | ||
align-items: center; | ||
justify-content: center; | ||
min-height: $main-container-available-height; |
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.
My two cents, min-height should be dynamic based on a prop.
This min-height is fine if we have the presence of the header and footer.
But if the error occurs at the layout level we will have neither header nor footer, causing the container height to not grow to the full height of the viewport, in that case the min-height should be 100vh I believe
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 good to me!
const footer = document.querySelector('footer') | ||
const newMinHeight = header && footer ? '$main-container-available-height' : '100vh' | ||
document.documentElement.style.setProperty('--error-min-height', newMinHeight) | ||
|
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 would avoid running natives javascript dom selectors to find elements by their tag name or even by a class name, these might change and might not even be the only ones you find.
Take a look at this, is from my lazy imports PR, see this tsx file and this scss one.
So then you only need to pass that prop to the ErrorPage on the root layout path object.
{
path: '/',
element: <Layout />,
errorElement: <ErrorPage fullViewport />,
children: [
...rest
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.
OK, I'll follow the example. Thank you!
Sorry if I approved this prematurely, I thought it was ready for QA, but maybe I misread the ticket! |
Don't worry, is just that I left a comment without a formal review I think |
What this PR does / why we need it:
In some cases, unexpected errors may occur in the application. To manage these effectively, we need a consistent error-handling mechanism in the UI. This not only ensures that error messages are displayed to users on the page but also enables developers to track errors by viewing detailed messages in the console.
Which issue(s) this PR closes:
Special notes for your reviewer:
The error page didn't do component test and e2e test.
Component test: we log errors using the hook useRouteError, which must be used within a data router. However, the component test used memory router.
e2e test: we didn't find a way to throw an error in the test environment to trigger the page. Cypress always caught the errors we throw.
Suggestions on how to test this:
Step 1: Run the Development Environment
Step 2: Test the feature
Since we could only test it manually, please edit some codes in Dataverse-frontend repository.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
No
Additional documentation: