-
Notifications
You must be signed in to change notification settings - Fork 22
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
#8980 Remove standalone mod api calls #9006
Conversation
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 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9006 +/- ##
==========================================
- Coverage 74.24% 74.22% -0.03%
==========================================
Files 1332 1347 +15
Lines 40817 41684 +867
Branches 7634 7792 +158
==========================================
+ Hits 30306 30938 +632
- Misses 10511 10746 +235 ☔ View full report in Codecov by Sentry. |
{ modId }, | ||
{ | ||
// Force-refetch the latest data for mod definition before activation. (Because the user might | ||
// have already had the Extension Console open when the mod was updated.) |
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.
👍
/** | ||
* Optionally force a specific mod component id to be assigned to the mod. If the mod definition has multiple | ||
* components, an error will be thrown. | ||
* @since 2.0.6 |
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, @BLoe this looks like the code you were talking about that was introduced to make it easy to ease off standalone mod components? Was thinking to myself that I didn't recognize the forceModComponentId
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.
Yes, that was introduced recently
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'm fine with deferring to others on the conversation regarding removing the useMods tests, with a follow-up ticket to test that hook
No loom links were found in the first post. Please add one there if you'd like to it to appear on Slack. Do not edit this comment manually. |
@mnholtz There is so much change coming soon to the mods page and this hook, that I didn't think it would be worth writing new tests for what is essentially a "transient" state in the code base. Once the Mods Page changes settle down, I plan on going back through and adding test coverage everywhere that it makes sense at that point. |
What does this PR do?
For more information on our expectations for the PR process, see the
code review principles doc