#8293 Unavailable overlay for sidebar forms and temp panels on nav #8351

fungairino · 2024-04-26T00:46:20Z

What does this PR do?

Closes Slice 2: Show “Panel is no longer available” overlay on info panels/forms #8293
Fixes Sidebar form doesn't submit #8284
Includes a fix for the notification sound for the npm run watch command.
Includes some e2e lint error fixes

Discussion

Perhaps we don't need a feature flag for this feature. Thoughts?

Demo

Screen.Recording.2024-04-26.at.4.42.19.PM.mov

TODO

Get the close button working
Add to strict null checks
Playwright test

Checklist

Add jest or playwright tests and/or storybook stories
Designate a primary reviewer @BLoe

twschiller · 2024-04-26T09:58:55Z

src/sidebar/ConnectedSidebar.tsx

@@ -98,7 +104,21 @@ const ConnectedSidebar: React.VFC = () => {
  const listener = useConnectedListener();
  const sidebarIsEmpty = useSelector(selectIsSidebarEmpty);

-  // `useAsyncEffect` will run once on component mount since listener and formsRef don't change on renders.
+  const navigationListener = useCallback(


For readability, I'd recommend moving this and the listener registration to separate hook because it's an independent concern. As-is, it's weaved in with the other listener registration code. E.g., useHostNavigationInvalidationListener()

twschiller · 2024-04-26T10:00:10Z

src/sidebar/ConnectedSidebar.tsx

@@ -132,10 +152,13 @@ const ConnectedSidebar: React.VFC = () => {
    // To avoid races with panel registration, listen after reserving the initial panels.
    addListener(listener);

+    browser.webNavigation.onBeforeNavigate.addListener(navigationListener);


You'll probably need to add mocks for tests.

Also, instead of putting the MV3 condition in the handler, I'd recommend conditionally adding the listener instead

twschiller · 2024-04-26T10:00:58Z

src/sidebar/Tabs.module.scss

@@ -24,6 +24,7 @@

 .tabContainer {
  flex-wrap: nowrap;
+  position: relative;


What's the change for? Will this break any overflow/scroll logic inside panels?

This is needed so that the blur overlay only covers the panel content area, and does not cover the whole sidebar (including the tabs and logo).
As far as I can tell, it has no other impact on UX such as overflow and scrolling. I can make a note here.

I'm far from a CSS expert, but it makes sense to me that a "container" element would have position: relative, right? As in this exact use-case, elements inside the "container" should expect to be able to declare their position relative to the container rather than the entire sidebar window.

twschiller · 2024-04-26T10:29:39Z

src/sidebar/unavailableOverlay.module.scss

+}
+
+.modal-button {
+  border-radius: 4px;


@BrandonPxBx is this a different border radius than other buttons in the UI? Is that intentional or just an artifact of the Figma design?

@twschiller Most likely an artifact of the Figma components not matching production components. What's the normal radius on our buttons in the code base? I'll update the Figma components to match.

The walkthrough modal here (accessed from the quickbar, "Learn: Open the page Editor"):

Is dynamic for some reason, and is around 4px border radius

Other modal components in our storybook are at 4.8px
https://pixiebrix-extension-storybook.vercel.app/?path=/story/components-confirmationmodal--custom-content

Looks like there's stuff all over the place in the product.

Default bootstrap settings

Large - 0.3 rem (4.8 px @ 16px default font size)

Small - 0.2 rem (3.2 px @ 16px default font size)

Some other buttons

Refresh icon button on Integration Select filed in Page Editor - 4px hardcoded

Refresh button on the Preview Pane in the Data Panel on a Trigger Starter Brick - 3.2px hard coded

Icon buttons on the mod list panel - 4px hardcoded

Add property in K:V data type input - 4px hardcoded

Because the "system" in Figma is based on the Bootstrap theme, but we cannot use REM values as measurements in Figma, I think there's been some lost in translation throughout the design life cycle.

TL;DR - Lets go with default bootstrap of 0.2 rem on the small buttons.

What about the border-radius for the modal dialog container? In the mock-up you used 12px, but that doesn't seem to line up with the other modals and bootstrap default.

twschiller · 2024-04-26T13:03:39Z

src/sidebar/UnavailableOverlay.tsx

+  <div className={styles["unavailable-overlay"]}>
+    <div
+      className={cx("modal show position-static w-auto h-auto")}
+      style={{ display: "block", marginTop: "10vh" }}


NIT: why not use CSS styles file here?

I can move it into the scss file, just missed it

twschiller · 2024-04-26T13:04:16Z

src/sidebar/UnavailableOverlay.tsx

+      style={{ display: "block", marginTop: "10vh" }}
+    >
+      <Modal.Dialog
+        size={"sm"}


NIT: { not required for string constants

twschiller · 2024-04-26T13:05:29Z

src/sidebar/UnavailableOverlay.tsx

+import { Button, Modal } from "react-bootstrap";
+
+const UnavailableOverlay = () => (
+  <div className={styles["unavailable-overlay"]}>


NIT: IIRC, in other places we write using unavailableOverlay so we can just do styles.unavailableOverlay. Any reason to prefer kebab-case?

I've seen some inconsistent naming pattern in scss files between camel and dash case for class names, but I'm happy to switch back to camel case

IMO - CSS Modules should use camel case because the classes are used directly in js code, and because these values don't actually get written to the DOM, and regular scss files should prefer kebab case because that's what will actually be added to the DOM attribute.

twschiller · 2024-04-26T13:07:25Z

src/sidebar/UnavailableOverlay.tsx

+    >
+      <Modal.Dialog
+        size={"sm"}
+        className={cx(styles["modal-dialog"], "shadow text-center")}


NIT: I prefer not mixing utility classes and class names for a component because it splits the concern across 2 locations. Although we don't have a great way to reference the utility classes in the CSS 🤷 so perhaps the code re-use better for non-trivial classes like shadow

Yea, my concern is re-usability and consistency. I'll see if it looks better if I move it into the styles file.

We mix these in a lot of places already, along with className pass-through

The general policy I follow is: "don't mix on the same component, but OK to use both CSS module and utility class in the same file". But again, just a NIT

twschiller · 2024-04-26T13:08:41Z

src/types/sidebarTypes.ts

+   * Determines if the panel cannot be displayed for the current tab. Used
+   * to show an overlay over the panel to indicate it is unavailable.
+   */
+  isUnavailable?: boolean;


NIT: adding @since 1.8.14 can be a good way for developers to get a sense of the type/API evolution. (And also, e.g., adding a comment that we added the field to account for MV3 side panel that persists across page redirects/navigation)

NIT: given that the value isn't persisted anywhere (so no migration is required), should we just make it required to simplify checks? In general, avoiding nullness where possible is probably "a good thing™"

In this case it's not so bad because false/null mean the same thing

Does it need to be designed as a negative boolean? Any chance we can go with isAvailable instead for simplicity's sake? Does that make the logic worse or something?

It's easier to make this not required since hunting down all the definitions and factories and updating them to include an explicit isUnavailable attribute is somewhat tedious and slightly impacts readability. As you noted, it's almost functionally the same, except for cases when for some reason, we would want to check for the existence of the attribute itself in the type.

That makes sense to me as long as it doesn't make strict-null-checks substantially harder in some way

twschiller · 2024-04-26T13:09:30Z

webpack.config.mjs

@@ -203,7 +203,7 @@ const createConfig = (env, options) =>
        }),

      // Only notifies when watching. `zsh-notify` is suggested for the `build` script
-      options.watch &&
+      !isProd(options) &&


…el-is-no-longer-available-overlay-on-info-panelsforms

twschiller · 2024-04-26T23:52:44Z

src/store/sidebar/thunks/removeFormPanel.ts

+ * Close form panels
+ * @param nonces panel nonces
+ */
+async function closeForms(nonces: UUID[]): Promise<void> {


I don't think calling cancelForm is necessary (or desireable) given that the forms won't exist on the form controller after the page redirect/navigation (because there will be a new content script and formController is a module level variable)

In that case, it will be a no-op. See:

pixiebrix-extension/src/platform/forms/formController.ts

Line 132 in bdab1e6

* Cancel some forms. Is a NOP if a form is no longer registered.

I guess we could check the panel and if panel.isUnavailable then we skip calling closeForms

The form/panel registration logic already needs to handle invalid/unregistered nonces in order to pass strict null checks, so we can be confident that a bug will not be introduced here in the future. Adding the if-check here means that this thunk is now essentially tightly coupled with behavior in other parts of the redux slice, with no change in functionality of this thunk. I think we should remove the if-check here and let cancelForm no-op, since it's simpler and less-coupled overall code, with no increased risk.

@twschiller This is also used when the user clicks the close button on the panel, not just on navigation/with unavailable panels.

codecov · 2024-04-26T23:55:33Z

Codecov Report

Attention: Patch coverage is 73.84615% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 73.51%. Comparing base (6608569) to head (ab01cd6).

Files	Patch %	Lines
src/sidebar/ConnectedSidebar.tsx	66.66%	5 Missing ⚠️
src/store/sidebar/sidebarSlice.ts	54.54%	5 Missing ⚠️
src/sidebar/Tabs.tsx	66.66%	2 Missing ⚠️
src/sidebar/TemporaryPanelTabPane.tsx	60.00%	2 Missing ⚠️
src/sidebar/UnavailableOverlay.tsx	85.71%	1 Missing ⚠️
src/sidebar/connectedTarget.tsx	0.00%	1 Missing ⚠️
src/store/sidebar/thunks/removeFormPanel.ts	93.33%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8351   +/-   ##
=======================================
  Coverage   73.50%   73.51%           
=======================================
  Files        1330     1332    +2     
  Lines       41142    41200   +58     
  Branches     7657     7665    +8     
=======================================
+ Hits        30241    30287   +46     
- Misses      10901    10913   +12

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

fungairino · 2024-04-27T00:15:35Z

end-to-end-tests/tests/bricks/sidebarEffects.spec.ts

@@ -32,7 +32,6 @@ test.describe("sidebar effect bricks", () => {
    await page.goto("/");

    // Ensure the page is focused by clicking on an element before running the keyboard shortcut, see runModViaQuickbar
-    await page.getByText("Index of  /").click();


I refactored the runModViaQuickBar method so we don't have to have these click calls before it.

fungairino · 2024-04-27T00:16:06Z

end-to-end-tests/tests/runtime/sidebarController.spec.ts

@@ -70,7 +70,7 @@ test.describe("sidebar controller", () => {

    // The focus dialog should not be shown in the iframe. Check after checking the top-level frame
    // because it's a positive check for the dialog being shown.
-    await expect(frame.getByRole("button", { name: "OK" })).not.toBeVisible();
+    await expect(frame.getByRole("button", { name: "OK" })).toBeHidden();


Unrelated lint error fixed

fungairino · 2024-04-27T00:17:46Z

end-to-end-tests/tests/telemetry/errors.spec.ts

+async function waitForBackgroundPageRequest(
+  context: BrowserContext,
+  extensionId: string,
+  errorServiceEndpoint: string,
+) {


refactored this method in the process of fixing an eslint error for no conditionals

fungairino · 2024-04-27T00:18:37Z

end-to-end-tests/utils.ts

@@ -161,6 +164,15 @@ export async function waitForSelectionMenuReadiness(page: Page) {
  }).toPass({ timeout: 5000 });
 }

+// Waits for the quick bar to be ready to use
+async function waitForQuickBarReadiness(page: Page) {


Without this check, sometimes the quickbar would not open when immediately attempting to open it after loading a new page.

BLoe · 2024-04-29T14:53:39Z

src/sidebar/ConnectedSidebar.test.tsx

+      await waitForEffect();
+
+      // The navigation listener should be added for MV3
+      expect(
+        browser.webNavigation.onBeforeNavigate.addListener,
+      ).toHaveBeenCalledWith(expect.any(Function));


Does this have to use waitForEffect()? Could this be done instead as:

Suggested change

await waitForEffect();

// The navigation listener should be added for MV3

expect(

browser.webNavigation.onBeforeNavigate.addListener,

).toHaveBeenCalledWith(expect.any(Function));

await waitFor(() => {

// The navigation listener should be added for MV3

expect(

browser.webNavigation.onBeforeNavigate.addListener,

).toHaveBeenCalledWith(expect.any(Function));

});

Turns out it's not really even needed

BLoe · 2024-04-29T15:25:55Z

src/store/sidebar/thunks/removeFormPanel.ts

+  const topLevelFrame = await getConnectedTarget();
+  cancelForm(topLevelFrame, ...nonces);


Any particular reason to extract these two lines into a separate, local (private) function here? Why not just inline this?

Mostly just following the pattern set by the other removeTemporaryPanel.ts thunk, but I agree it can be inlined

BLoe · 2024-04-29T15:27:28Z

src/testUtils/factories/sidebarEntryFactories.ts

@@ -38,11 +38,13 @@ const activateModPanelEntryFactory = define<ModActivationPanelEntry>({
    },
  ],
  heading: (n: number) => `Activate Mods Test ${n}`,
+  isUnavailable: false,


Just curious, if this isn't required, then we can probably leave these values off the factories, right?

Yes, we could.

BLoe

I'm not the one who should approve the design/UX parts here, but this is looking good from a code perspective to me 👍 Happy to unblock if needed but will defer to someone else here to approve based on the design/copy.

fungairino · 2024-04-29T15:53:39Z

I'm not the one who should approve the design/UX parts here

@BrandonPxBx has reviewed the final UX!

github-actions · 2024-04-29T17:06:14Z

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.

Unavailable overlay for sidebar forms and temp panels on nav

b109943

fungairino self-assigned this Apr 26, 2024

fungairino linked an issue Apr 26, 2024 that may be closed by this pull request

Slice 2: Show “Panel is no longer available” overlay on info panels/forms #8293

Closed

fungairino changed the title ~~Unavailable overlay for sidebar forms and temp panels on nav~~ #8293 Unavailable overlay for sidebar forms and temp panels on nav Apr 26, 2024

vercel bot deployed to Preview April 26, 2024 00:47 View deployment

fungairino requested review from BLoe and grahamlangford April 26, 2024 00:50

twschiller added enhancement New feature or request mv3 labels Apr 26, 2024

twschiller reviewed Apr 26, 2024

View reviewed changes

close button works, added tests

ba987c4

vercel bot deployed to Preview April 26, 2024 23:48 View deployment

Merge remote-tracking branch 'origin/main' into 8293-slice-2-show-pan…

e4dc7e3

…el-is-no-longer-available-overlay-on-info-panelsforms

vercel bot deployed to Preview April 26, 2024 23:49 View deployment

twschiller reviewed Apr 26, 2024

View reviewed changes

fixing lint errors

0c94e51

fungairino marked this pull request as ready for review April 27, 2024 00:14

vercel bot deployed to Preview April 27, 2024 00:14 View deployment

fungairino commented Apr 27, 2024

View reviewed changes

wip

0832c12

vercel bot deployed to Preview April 27, 2024 00:35 View deployment

BLoe reviewed Apr 29, 2024

View reviewed changes

PR comments

ab01cd6

vercel bot deployed to Preview April 29, 2024 15:54 View deployment

fungairino requested a review from BLoe April 29, 2024 16:49

BLoe approved these changes Apr 29, 2024

View reviewed changes

fungairino merged commit 53f84ce into main Apr 29, 2024
22 checks passed

fungairino deleted the 8293-slice-2-show-panel-is-no-longer-available-overlay-on-info-panelsforms branch April 29, 2024 17:20

twschiller added this to the 1.8.14 milestone Apr 30, 2024

		const topLevelFrame = await getConnectedTarget();
		cancelForm(topLevelFrame, ...nonces);

#8293 Unavailable overlay for sidebar forms and temp panels on nav #8351

#8293 Unavailable overlay for sidebar forms and temp panels on nav #8351

Conversation

fungairino commented Apr 26, 2024 • edited

What does this PR do?

Discussion

Demo

TODO

Checklist

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BLoe Apr 26, 2024 • edited

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

BrandonPxBx Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BrandonPxBx Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BLoe Apr 26, 2024 • edited

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BLoe Apr 26, 2024 • edited

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Apr 26, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BLoe left a comment

Choose a reason for hiding this comment

fungairino commented Apr 29, 2024 • edited

github-actions bot commented Apr 29, 2024

fungairino commented Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

BLoe Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

BrandonPxBx Apr 26, 2024 •

edited

BrandonPxBx Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

BLoe Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

BLoe Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

twschiller Apr 26, 2024 •

edited

codecov bot commented Apr 26, 2024 •

edited

fungairino commented Apr 29, 2024 •

edited