-
Notifications
You must be signed in to change notification settings - Fork 843
Scan Package: Migrate Components #41654
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
Conversation
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
5142a6c to
643217f
Compare
af7e208 to
dff3b78
Compare
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryCannot generate coverage summary while tests are failing. 🤐 Please fix the tests, or re-run the Code coverage job if it was something being flaky. |
643217f to
93a48e4
Compare
08668ad to
e62f892
Compare
878df52 to
60f0892
Compare
95f24d4 to
67e1bde
Compare
Code Coverage SummaryCoverage changed in 3 files.
14 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
67e1bde to
0f9932e
Compare
| export type { ButtonProps } from './components/button/types.js'; | ||
| export { default as LoadingPlaceholder } from './components/loading-placeholder/index.js'; | ||
| export { default as TermsOfService } from './components/terms-of-service/index.js'; | ||
| export { default as Badge } from './components/badge/index.js'; |
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.
Exporting Badge here so threat components can use it from their new home in the scan package.
anomiex
left a comment
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 reasonable to me from a Garage perspective. Haven't tested though.
| @@ -0,0 +1,4 @@ | |||
| Significance: major | |||
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 is! Thanks!
7d122ad to
a44f790
Compare
changelog changelog Update changelog Update changelog changelog
a44f790 to
3a76ec3
Compare
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.
Changes look good, and the component renders well in Protect and handle varying versions when compatibility testing.
One thing I will note though, and this may be intentional but there is now a slight variation in styling for the SeverityBadge from trunk when compared against the feature branch:
This is the updated badge design from the Protect Meets Core project, should be fine to merge into trunk early IMO 👍 |


Proposed changes:
@automattic/jetpack-componentsand into@automattic/jetpack-scan.@wordpress/dataviews@automattic/jetpack-scanOther information:
Jetpack product discussion
peb6dq-3ta-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions: