-
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
Update the block directory state to store the installing status per block id. #22881
Conversation
Size Change: +784 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
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 just noticed this yesterday, thanks for fixing it. It's working out in testing 👍
Some comments, but they're pretty minor.
packages/block-directory/src/components/downloadable-block-header/index.js
Show resolved
Hide resolved
packages/block-directory/src/components/downloadable-block-list-item/index.js
Outdated
Show resolved
Hide resolved
if ( ! state.blockManagement.isInstalling[ blockId ] ) { | ||
return false; | ||
} | ||
return state.blockManagement.isInstalling[ blockId ]; |
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.
You could simplify this like…
if ( ! state.blockManagement.isInstalling[ blockId ] ) { | |
return false; | |
} | |
return state.blockManagement.isInstalling[ blockId ]; | |
return !! state.blockManagement.isInstalling[ blockId ]; |
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.
Funny enough, I had it that way but switched it back since it diverges from the way its done in other selectors.
Additionally, I find it less 'readable' for people who don't understand JS quirks.
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.
huh, I found this pattern in a few other selectors. I don't have any strong preference, so this is good 👍
Noting: This needs more tests before merging. |
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 👍
Were you still planning on adding more tests? I think this is good to merge either way.
Description
When installing a block from the block directory the
isInstalling
state is shared across all the results. This means that if I install 1 block, all the buttons display the loading state.How has this been tested?
Screenshots
Types of changes
Checklist: