-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add more newspack blocks fixes #79272
Conversation
This PR does not affect the size of JS and CSS bundles shipped to the user's browser. Generated by performance advisor bot at iscalypsofastyet.com. |
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.
Thank you for taking care of this one so quickly, @noahtallen! 🙌
It looks great - let's give it a try! 🚀
The best way would be opening a couple of PRs, one that breaks it and one that doesn't, and demonstrating that CI will fail in one of them and succeed in the other.
fi | ||
|
||
set +e | ||
eslint_result=$(npx eslint-nibble --no-interactive --rule '@wordpress/i18n-text-domain' .) |
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 greatly appreciate we're using eslint-nibble
! It's much better and less fragile if we precisely fix only this rule and no others.
I did demonstrate this in a few commits above! You can see the failing checks ;) |
This reverts commit beb5e63dc15184b3cfd1b2ab6a541697f8a86dfe.
This reverts commit 75e2ee98b476817e81ca88214c61e8797886aabc.
Co-authored-by: Marin Atanasov <[email protected]>
13013ba
to
af2b5d7
Compare
This PR modifies the release build for editing-toolkit To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-mMA-p2 |
Verifying it again: #79385 |
Related to: #79266
Proposed Changes
Follow-up from #79097 (comment), to add extra checks for ETK's text domains.
The new script only succeeds if all of the following are true:
(This phpcs call uses a custom config which only includes the i18n rule.)
It fails otherwise.
Proof
Proof that it catches issues with JS textdomain in CI: https://teamcity.a8c.com/buildConfiguration/calypso_WPComPlugins_EditorToolKit/10371617?hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&showLog=10371617_801_752&logView=flowAware
Proof that it catches issues with PHP textdomain in CI: https://teamcity.a8c.com/buildConfiguration/calypso_WPComPlugins_EditorToolKit/10371663?buildTab=overview&hideProblemsFromDependencies=false&hideTestsFromDependencies=false&expandBuildChangesSection=true&showLog=10371663_1390_962&logView=flowAware
Testing Instructions
yarn distclean
thenyarn install
./bin/verify-textdomain.sh
-- it should fail since the blocks are not synced and JS bundles don't exist.yarn build
apps/editing-toolkit/editing-toolkit-plugin/newspack-blocks/synced-newspack-blocks/blocks/homepage-articles/edit.js
__
.