-
-
Notifications
You must be signed in to change notification settings - Fork 234
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: fixed crash on list contributor grid when in card view changing page size #2241
Conversation
✅ Deploy Preview for oss-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for design-insights ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -97,6 +97,7 @@ | |||
"react-day-picker": "^8.7.1", | |||
"react-dom": "^18.2.0", | |||
"react-emoji-render": "^2.0.1", | |||
"react-error-boundary": "^4.0.11", |
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.
react-error-boundary will allow us to have a fallback view if we wrap a component with it that crashes.
}), | ||
fetchApiData<DBList>({ path: `lists/${listId}`, bearerToken, pathValidator: validateListPath }), | ||
]); | ||
const { data: list, error } = await fetchApiData<DBList>({ |
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've removed server-side rendering contributor list data and only kept critical stuff to load the page, e.g. list info.
Something related to the initial data for the contributor list is what caused the issue with the card view. Still trying to work out why, but regardless it makes sense to only load this client-side going forward as mentioned in the PR description.
range={String(range) ?? "30"} | ||
/> | ||
<ErrorBoundary | ||
fallback={<div className="grid place-content-center">Error loading the list of contributors</div>} |
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 went with a barebones fallback error message @isabensusan, but going forward a bit of styling for this would be nice as we can use this for graphs or other data driven components. It can be in another PR though.
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.
LGTM codewise 👍
### [1.78.1-beta.1](v1.78.0...v1.78.1-beta.1) (2023-12-01) ### 🐛 Bug Fixes * fixed crash on list contributor grid when in card view changing page size ([#2241](#2241)) ([f6bc793](f6bc793))
## [1.79.0](v1.78.0...v1.79.0) (2023-12-05) ### 🍕 Features * upgraded to Storybook 7.6.3 ([#2244](#2244)) ([a2ae11a](a2ae11a)) ### 🐛 Bug Fixes * fixed crash on list contributor grid when in card view changing page size ([#2241](#2241)) ([f6bc793](f6bc793)) * now most used languages are truncated to a limit of two ([#2238](#2238)) ([97ce66e](97ce66e)) * now the highlight input form closes only if you press ESC or click the close button ([e56ea73](e56ea73)) * now the highlight input form closes only if you press ESC or click the close button ([#2257](#2257)) ([1b94421](1b94421))
Description
I've added react-error-boundary to the project that allows us to catch errors at the component level and display a fallback component if an error does occur.
Aside from that, it appears the initial contributor list data was causing issues with the component. This was working before, so not sure why removing it now fixes the issue, but as we get larger lists of contributors, it makes sense to offload this to the client-side only, much like the fix in #2215.
What type of PR is this? (check all applicable)
Related Tickets & Documents
Fixes #2240
Mobile & Desktop Screenshots/Recordings
Before
The before view is running the dev server which is why you get the detailed error. If it was production, it would be the classic Next.js black screen of death.
CleanShot.2023-11-30.at.22.14.30.mp4
After
CleanShot.2023-11-30.at.19.55.42.mp4
Added tests?
Added to documentation?
[optional] Are there any post-deployment tasks we need to perform?
[optional] What gif best describes this PR or how it makes you feel?