-
Notifications
You must be signed in to change notification settings - Fork 23
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
#9209: eliminate new standalone mod components from Page Editor UI via "unsaved" mods #9219
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9219 +/- ##
==========================================
+ Coverage 74.24% 74.75% +0.51%
==========================================
Files 1332 1368 +36
Lines 40817 42356 +1539
Branches 7634 7914 +280
==========================================
+ Hits 30306 31665 +1359
- Misses 10511 10691 +180 ☔ View full report in Codecov by Sentry. |
Playwright test resultsDetails Open report ↗︎ Flaky testschrome › tests/pageEditor/addStarterBrick.spec.ts › Add starter brick to mod Skipped testschrome › tests/regressions/doNotCloseSidebarOnPageEditorSave.spec.ts › #8104: Do not automatically close the sidebar when saving in the Page Editor |
@@ -75,7 +75,8 @@ describe("useCreateModFromModComponent", () => { | |||
}); | |||
}); | |||
|
|||
it("does not throw an error if the mod fails the compareModComponentCounts check", async () => { | |||
// eslint-disable-next-line jest/no-disabled-tests -- Unclear if we need this test anymore | |||
it.skip("does not throw an error if the mod fails the compareModComponentCounts check", async () => { |
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.
Ideally we should either validate that we do need these now or delete them. Otherwise, I think they're just going to end up being left skipped
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'll update the comment, I meant that I think I need to finish the entire project and then see if it still makes sense to have this test and update it to work, or if we should delete it.
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.
Those tests are no longer relevant because the hook doesn't call the methods because there's no original/"source" mod
What does this PR do?
Remaining Work
Product/Design micro-decisions
cc @Pashaminkovsky
Future Work
id=description
field is contained within both forms: CreateModModal causes multiple DOM elements with same id #9238For more information on our expectations for the PR process, see the
code review principles doc