-
Notifications
You must be signed in to change notification settings - Fork 219
Conversation
The release ZIP for this PR is accessible via:
Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
Size Change: +1.27 kB (0%) Total Size: 1.08 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.
LGTM! 🚀
The code looks good, I do not see any issues there.
With manual testing, I’ve confirmed the following:
- On
trunk
, the Products block is available in the inserter when runningnpm run build
but not available when runningnpm start
. - On this branch (
fix/add-is-site-editor-page
), the Products block is available in the inserter in both of the above cases.
I tried to create some E2E tests around this function without success. Can we create a follow-up issue to do during the cooldown?
Seems reasonable to me. IMO, it'd be better to roll out the fix for the regression that’s handled in this PR, as opposed to running the risk of having the Products block disappear in the next release.
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.
Nice job!
Tested both: npm run start
and npm run build
in both areas: Editor and post/page Editor.
All works good 👍
I created the issue about E2E tests woocommerce/woocommerce#42370. I will merge the PR 🚀 Thanks for the review! |
44810bb
to
c1486a6
Compare
c1486a6
to
d0aaa83
Compare
d0aaa83
to
4a41c32
Compare
Hi folks, the E2E tests (💘) caught an issue with the previous logic. When the Products block is added to a template, it has to have the "Inherit query from template" option enabled. With 670893e, it didn't happen. I refactor the logic with 4a41c32. Furthermore, I noticed another error. I think that it is fine to merge this PR, but just as a temporary fix. |
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.
This is testing well on my end and code looks good. Thanks for investigating and fixing this issue, @gigitux! I left one question but it's not something that needs to be changed, just one thing I'm not sure to understand.
Besides that, some days ago I worked-around a similar issue in the Mini-Cart block: #9442. I think it makes sense to update that code to use the same util you introduce here, do you agree? (I can take care of it, but wanted to know your opinion)
let isBlockRegistered = false; | ||
subscribe( () => { | ||
if ( ! isBlockRegistered ) { | ||
isBlockRegistered = true; |
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.
Out of curiosity, why do we need to keep track of isBlockRegistered
in the post editor but not in the site editor?
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.
Oh, I saw it now, not having it causes the Site Editor to freeze.
Yes, I think that it makes sense to update it! |
This PR fixes an issue introduced with #9386. Since we are currently importing the redux store
core/edit-site
, the store is initialized (as an empty object), and for this reason some checks that we are currently failing. Also, I added the filerevert-button.ts
in thesideEffect
array in the package.json, so this file will be run on production builds too.This PR adds the
isSiteEditorPage
that returns a boolean to check whether the current page is a site editor.I tried to create some E2E tests around this function without success. Can we create a follow-up issue to do during the cooldown?
Other Checks
Testing
Automated Tests
User Facing Testing
WooCommerce Visibility