-
Notifications
You must be signed in to change notification settings - Fork 798
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
Slideshow: Add unit tests and fixtures to validate block content parsing #18967
Conversation
Caution: This PR has changes that must be merged to WordPress.com |
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 If you are an automattician, once your PR is ready for review add the "[Status] Needs Team review" label and ask someone from your team review the code. jetpack plugin:
|
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 ran the tests locally and all passed, and also manually tested the Slideshow block on a sandbox and a local Jetpack install.
I did notice an Each child in a list should have a unique "key" prop.
error in the console in my local env for the Slideshow, but that was not caused by this PR. From a quick glance I'm not sure why it's complaining - maybe worth following up later?
LGTM
…'t have an id yet
I just added a quick fix for this - was happening when files were uploaded so didn't yet have an id, so now uses the blob url as a key in these cases. |
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.
Even better -- re-tested with the fix and it looks good to me, no more console errors. Thanks!
<BlockControls> | ||
{ !! images.length && ( | ||
<ToolbarGroup> | ||
<ToolbarItem> |
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 see you've removed ToolbarItem
. I added it in #18860 to address these changes:
https://make.wordpress.org/core/2020/11/18/changes-to-toolbar-components-in-wordpress-5-6/
Do you think we should get rid of it, or can we keep it here?
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 mistake here. When running through Jest ToolbarItem
was erroring as undefined and I read some out-of-date docs which seemed to indicate it isn't needed. You are correct though that it should be there.
I have added it back and the reason it shows as undefined in jest tests is because the @wordpress/components
dependency in the jetpack project is 9.2.6
which still as ToolbarItem
as __experimentalToolbarItem
.
I assume that the build process is pulling in a newer dependency somehow via the webpack '@automattic/calypso-build/webpack.config.js'
. I tried forcing a newer version of @wordpress/components
in package.json
but this made things more broken. @brbrr, do you have any ideas on how to get jest using the same dependency tree for the @wordpress
dependencies as the webpack builds? If you run yarn jest extensions/blocks/slideshow/test/controls.js
on this PR you will get the error about ToolbarItem
being undefined which doesn't happen on the webpack build.
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.
Ah right, I ran into the same issue in #18860, but had forgotten about it :/
I had tried to update our dependencies, but was running into other issues; I ended up reverting:
b91bb2a
We'll need to merge #16763 before we can do anything. Until then, maybe we can remove ToolbarItem
like you had done, with a note like the one I left in b91bb2a ?
Unless @brbrr has a better idea :)
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.
At this point I have just removed the single test for the toolbar, which was only doing a very basic 'did it render' sort of test, so not critical, and have added a comment in the spec file to indicate why there is no test for this component.
block build job currently broken on wpcom so will merge diff next week |
diff deployed rWP222707 |
Changes proposed in this Pull Request:
Jetpack product discussion
See p1HpG7-b3G-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
yarn jest extensions/blocks/slideshow/test/controls.js
&yarn jest extensions/blocks/slideshow/test/edit.js
&yarn jest extensions/blocks/slideshow/test/validate.js
in jetpack dir and make sure they all passProposed changelog entry for your changes: