-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Renders DataForm
component only when data has been fetched
#67694
Renders DataForm
component only when data has been fetched
#67694
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +15 B (0%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
Flaky tests detected in b8c5361. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12200871013
|
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.
It's really not clear to me why this prop has to be in the DataForm component instead of just not rendering the component from the wrapper.
Thanks for the review! I don't have a strong preference, but for gutenberg/packages/dataviews/README.md Lines 334 to 336 in b8c5361
Mostly, I added it for “consistency”.
I'm okay with this approach, too. |
I don't think we manage very well the loading states in DataViews. Said that, the |
Yes, in DataViews, the component is not just about the "table" or the "grid", it's also about the filters... I do think the current isLoading in dataviews is not great, it should probably just be at the top level to be honest outside the component but better loading indicators... is probably needed later especially when switching config... |
DataForm
component only when data has been fetched
…aform-improve-loading-state-management
Thanks for your precious feedback (as always). I updated the PR! |
…ss#67694) Co-authored-by: gigitux <[email protected]> Co-authored-by: youknowriad <[email protected]> Co-authored-by: oandregal <[email protected]>
What?
This PR renders
DataForm
component only when data has been fetched.Why?
We experienced runtime crashes when certain fields expected defined data but received a boolean value (
false
). This occurred becausegetEditedEntityRecord
was still fetching data and returnedfalse
as a fallback.Instead, according to type definition, fields expect to receive the
Item
:https://github.com/WordPress/gutenberg/blob/e4df49ffc87d7f2076d4ea7e0e8bc6698bcc60ff/packages/dataviews/src/types.ts#L549-L555
To avoid runtime crashes,
DataForm
component needs to run only when data has been fetched.How?
Testing Instructions
console.log(data)
on this lineScreen.Capture.on.2024-12-06.at.15-45-37.mov
Screen.Capture.on.2024-12-06.at.15-45-37.mov