#8228: run show sidebar in top-level frame #8299
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8299 +/- ##
==========================================
- Coverage 73.50% 73.50% -0.01%
==========================================
Files 1330 1330
Lines 41108 41113 +5
Branches 7637 7636 -1
==========================================
+ Hits 30218 30220 +2
- Misses 10890 10893 +3 ☔ View full report in Codecov by Sentry. |
twschiller
changed the title
|
|
||
| const HIDE_SIDEBAR_EVENT_NAME = "pixiebrix:hideSidebar"; | ||
|
|
||
| /** |
There was a problem hiding this comment.
Moved module variables to top of file
| test.describe("sidebar controller", () => { | ||
| test("show sidebar uses top-level frame", async ({ page, extensionId }) => { |
There was a problem hiding this comment.
nit: on naming tests, where possible I think we should frame the test from the perspective of the user, so in this case, maybe something like "can show sidebar and displays user interaction modal if needed"?
| // eslint-disable-next-line playwright/no-wait-for-timeout -- match wait in the mod | ||
| await page.waitForTimeout(8000); | ||
|
|
||
| // Focus dialog should be visible in the top-level frame | ||
| await expect(page.getByRole("button", { name: "OK" })).toBeVisible(); |
There was a problem hiding this comment.
nit: I think you should be able to get rid of the waitForTimeout by extending the timeout in the toBeVisible call.
| // eslint-disable-next-line playwright/no-wait-for-timeout -- match wait in the mod | |
| await page.waitForTimeout(8000); | |
| // Focus dialog should be visible in the top-level frame | |
| await expect(page.getByRole("button", { name: "OK" })).toBeVisible(); | |
| // Focus dialog should be visible in the top-level frame | |
| await expect(page.getByRole("button", { name: "OK" })).toBeVisible({timeout: 8000}); |
There was a problem hiding this comment.
In this case, I prefer the waitForTimeout there's an explicit sleep/wait in the mod. So it's matching that wait/sleep brick
|
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. |
What does this PR do?
Remaining Work
Demo
@pixies/test/frame-sidebar-actions: see the "Show Sidebar after Wait" action that it shows the user gesture dialog on the top-level tabFuture Work
sidePanelUtilsandsidebarControllercontain some duplicate logic that we might want to clean up/clarify in the future. For example, theisSidePanelOpenmethodChecklist