#5078: support showing panel as popover #5023

twschiller · 2023-01-15T15:56:57Z

What does this PR do?

Part of PLACEHOLDER: support popover location for display brick #5078
https://www.notion.so/pixiebrix/Interactive-Panel-Modals-and-Popovers-9a9e61e44cc1421a8e0aa7f75302c96d#adb7c1a356774ac5a4a0057a739583f8
Adds a "hover" trigger using the hoverIntent library to capture more natural hover behavior
Slice 3.1: Add a showPopover method that renders a frame inside a popover
Slice 3.2: Make the Display Temporary Information Brick root-aware
Slice 3.3: Add location: popover option for the Display Temporary Information Brick

Known Issues

Scroll bar behavior
Jumpy UI when using with mouseover trigger
~~Improve placement of "x": N/A.~~ Using click outside event instead

Demo

https://www.loom.com/share/5661176ff0e245c9998473c44a66e5c0

Future Work

Not showing if toward the bottom of page? ('auto') not working?
Reduce flickering on render - likely need to use a stencil
Allow creator to specify position
Allow creator to specify width/height: initial, to reduce flickering and actual

Checklist

Add tests: I'll circle back tomorrow (Thursday) to write some tests
Designate a primary reviewer: @BLoe

Clear existing popovers Vendor hover intent; smooth out popover Namespace tooltips container

codecov · 2023-01-25T16:06:09Z

Codecov Report

Merging #5023 (5bd588e) into main (3619d1f) will decrease coverage by 0.08%.
The diff coverage is 45.16%.

❗ Current head 5bd588e differs from pull request most recent head 55aadeb. Consider uploading reports for the commit 55aadeb to get more accurate results

@@            Coverage Diff             @@
##             main    #5023      +/-   ##
==========================================
- Coverage   60.54%   60.47%   -0.08%     
==========================================
  Files         962      964       +2     
  Lines       29223    29316      +93     
  Branches     5608     5625      +17     
==========================================
+ Hits        17694    17728      +34     
- Misses      11529    11588      +59

Impacted Files	Coverage Δ
src/baseRegistry.ts	`85.00% <ø> (-0.15%)`	⬇️
...ocks/transformers/temporaryInfo/EphemeralPanel.tsx	`0.00% <0.00%> (ø)`
src/extensionPoints/menuItemExtension.ts	`64.26% <ø> (ø)`
src/pageEditor/exampleBlockConfigs.ts	`87.17% <ø> (ø)`
...c/pageEditor/tabs/trigger/TriggerConfiguration.tsx	`58.49% <ø> (ø)`
src/tinyPages/ephemeralPanel.tsx	`0.00% <ø> (ø)`
.../blocks/transformers/temporaryInfo/popoverUtils.ts	`11.42% <11.42%> (ø)`
...transformers/temporaryInfo/DisplayTemporaryInfo.ts	`71.01% <54.28%> (-10.99%)`	⬇️
src/extensionPoints/triggerExtension.ts	`58.86% <77.27%> (+0.52%)`	⬆️
...ansformers/temporaryInfo/temporaryPanelProtocol.ts	`89.58% <78.94%> (-7.39%)`	⬇️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

twschiller · 2023-01-25T16:15:41Z

src/vendors/hoverintent/hoverintent.js

@@ -0,0 +1,208 @@
+/*!


Vendored from http://briancherne.github.io/jquery-hoverIntent/ for module support

twschiller · 2023-01-25T22:46:07Z

src/baseRegistry.ts

@@ -90,12 +90,6 @@ export class Registry<
  }

  private notifyAll() {
-    console.debug(


This logging is super noisy

twschiller · 2023-01-25T22:52:22Z

src/blocks/transformers/temporaryInfo/DisplayTemporaryInfo.ts

@@ -89,65 +98,101 @@ class DisplayTemporaryInfo extends Transformer {
      title,
      body: bodyPipeline,
      location = "panel",
+      isRootAware = false,


Safe default

Why is the brick isRootAware() "safe default" true, but the transform() input param one is false?

See this PR for more details on the approach: #4910

The idea is that:

isRootAware is overridden so the runtime sees the brick as root aware

The brick exposes an isRootAware param, but it defaults to false in calls for backward compatability. It controls whether the brick actually considers the root or not in brick execution

In the JSONSchema, isRootAware defaults to "true", so new calls create in the Page Editor pass true

The Brick Configuration looks at the arg to determine which fields to show

twschiller · 2023-01-25T22:52:40Z

src/blocks/transformers/temporaryInfo/DisplayTemporaryInfo.ts

        default: "panel",
        description: "The location of the information (default='panel')",
      },
+      ...IS_ROOT_AWARE_BRICK_PROPS,


Taking same approach as other PR for upgrading the brick to be root aware

twschiller · 2023-01-25T22:53:23Z

src/blocks/transformers/temporaryInfo/DisplayTemporaryInfo.ts

+      target
+    )) as PanelPayload;
+
+    switch (location) {


Switch to a "switch" statement. The whitespace in the diff is bad

fregante · 2023-01-26T14:35:42Z

Can this be implemented with a shadowdom instead of iframe? It contains style and the JS still lives in the content script context, similarly to how the QuickBar and notifications work

BLoe

Just a few nits and minor questions, code seems good overall though 👍

I didn't check out the branch and do any local QA on the UX for this though.

BLoe · 2023-01-26T01:05:27Z

src/blocks/transformers/temporaryInfo/DisplayTemporaryInfo.ts

@@ -89,65 +98,101 @@ class DisplayTemporaryInfo extends Transformer {
      title,
      body: bodyPipeline,
      location = "panel",
+      isRootAware = false,


Why is the brick isRootAware() "safe default" true, but the transform() input param one is false?

BLoe · 2023-01-26T16:23:03Z

src/blocks/transformers/temporaryInfo/EphemeralPanel.tsx

  const [entry, isLoading, error] = useAsyncState(
    async () => getPanelDefinition(target, nonce),
    [nonce]
  );

+  useEffect(() => {
+    document.dispatchEvent(
+      new CustomEvent("@@pixiebrix/PANEL_MOUNTED", { detail: nonce })


Should probably put this event name in a constant in constants.ts with our other custom events there

BLoe · 2023-01-26T16:33:31Z

src/blocks/transformers/temporaryInfo/temporaryPanelProtocol.ts

@@ -123,7 +148,7 @@ export async function resolveTemporaryPanel(nonce: UUID, action: PanelAction) {
 * Cancel some temporary panels' deferred promises
 * @param nonces The nonces of the panels to reject with a CancelError
 */
-export async function cancelTemporaryPanels(nonces: UUID[]) {
+export async function cancelTemporaryPanels(nonces: UUID[]): Promise<void> {


nit: Make this a varargs param so we don't have to create arrays at all the call sites.

I prefer not to unless we're sure that we won't want to add an optional options arg on later. Personally, I'm not a big fan of var args for lists of things

BLoe · 2023-01-26T16:51:06Z

src/extensionPoints/triggerExtension.ts

+      // `hoverIntent` JQuery plugin has custom event names
+      $elements.off("mouseenter.hoverIntent");
+      $elements.off("mouseleave.hoverIntent");
+      $elements.hoverIntent({


Just checking, do we want to pass in a timeout delay here to hoverIntent options? Just want to make sure that calling the handler instantly on mouse out event is fine here for the UX on the popovers.

It's OK to call instantly, since the event has already fired at that point. There's no notion of a "hover end" trigger. We're really only using the hoverIntent for the "hover start"

BLoe · 2023-01-26T16:53:05Z

src/pageEditor/tabs/trigger/TriggerConfiguration.tsx

@@ -120,6 +120,7 @@ const TriggerConfiguration: React.FC<{
          <option value="dblclick">Double Click</option>
          <option value="blur">Blur</option>
          <option value="mouseover">Mouseover</option>
+          <option value="hover">Hover</option>


Are users going to be confused about the difference between Mouseover and Hover here?

Are users going to be confused about the difference between Mouseover and Hover here?

Probably? We'll need to clean up all out trigger names to better clarify what they are. E.g., most users won't have a way of knowing what "blur" means

github-actions · 2023-01-26T17:09:02Z

When the PR is merged, the first loom link found on this PR will be posted to #sprint-demo on Slack. Do not edit this comment manually.

twschiller · 2023-01-26T17:38:22Z

Can this be implemented with a shadowdom instead of iframe?

Sadly I don't think - we run into the same issue we do with forms. We want to isolate the content completely from the CSS (because out document builder uses bootstrap). And certain things like root font-size REM aren't isolated by the shadow dom

In the future, we might be able to get away with it by doing a custom build of bootstrap for forms/panels with CSS rules that can't be effected by any of the CSS props that leak into the shadow root

fregante · 2023-01-27T00:52:51Z

Ah yes it's a rem unit issue. Keeping a custom stylesheet where we replace this unit would probably be beneficial to avoid many of the iframes we have, making them faster to load and easier to handle

twschiller added the do not merge Merging of this PR is blocked by another one label Jan 15, 2023

twschiller changed the title ~~POC of Display Temporary Information for popovers~~ POC of Display Temporary Information via element popovers Jan 15, 2023

twschiller added the low priority label Jan 15, 2023

vercel bot deployed to Preview January 15, 2023 20:12 View deployment

twschiller self-assigned this Jan 15, 2023

twschiller added this to the 1.7.18 milestone Jan 25, 2023

[WIP] popver

977b33f

Clear existing popovers Vendor hover intent; smooth out popover Namespace tooltips container

twschiller force-pushed the feature/popover branch from 3d4c083 to 977b33f Compare January 25, 2023 15:52

vercel bot deployed to Preview January 25, 2023 15:56 View deployment

Fix header generation

0341f6e

vercel bot deployed to Preview January 25, 2023 16:02 View deployment

twschiller commented Jan 25, 2023

View reviewed changes

Auto-resize

21ebc08

twschiller removed low priority do not merge Merging of this PR is blocked by another one labels Jan 25, 2023

twschiller changed the title ~~POC of Display Temporary Information via element popovers~~ #5078: support showing panel as popover Jan 25, 2023

vercel bot deployed to Preview January 25, 2023 22:43 View deployment

twschiller commented Jan 25, 2023

View reviewed changes

Fix lint

5463b7a

vercel bot deployed to Preview January 25, 2023 22:50 View deployment

twschiller marked this pull request as ready for review January 25, 2023 22:51

twschiller assigned BLoe Jan 25, 2023

twschiller requested review from BLoe and BALEHOK and removed request for BLoe January 25, 2023 22:52

twschiller commented Jan 25, 2023

View reviewed changes

twschiller requested a review from fregante January 26, 2023 13:39

BLoe approved these changes Jan 26, 2023

View reviewed changes

Address PR comments

5bd588e

vercel bot deployed to Preview January 26, 2023 17:46 View deployment

Add coverage for popover

873e34f

vercel bot deployed to Preview January 26, 2023 18:07 View deployment

More test coverage

55aadeb

twschiller enabled auto-merge (squash) January 26, 2023 18:18

vercel bot deployed to Preview January 26, 2023 18:19 View deployment

twschiller merged commit 43b9e94 into main Jan 26, 2023

twschiller deleted the feature/popover branch January 26, 2023 18:24

fregante mentioned this pull request Feb 20, 2024

POC #5944: Just use shadow DOM instead of iframes #7616

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#5078: support showing panel as popover #5023

#5078: support showing panel as popover #5023

twschiller commented Jan 15, 2023 •

edited

Loading

codecov bot commented Jan 25, 2023 •

edited

Loading

twschiller Jan 25, 2023 •

edited

Loading

twschiller Jan 25, 2023

twschiller Jan 25, 2023

BLoe Jan 26, 2023

twschiller Jan 26, 2023 •

edited

Loading

twschiller Jan 25, 2023

twschiller Jan 25, 2023

fregante commented Jan 26, 2023

BLoe left a comment

BLoe Jan 26, 2023

BLoe Jan 26, 2023

BLoe Jan 26, 2023

twschiller Jan 26, 2023

BLoe Jan 26, 2023

twschiller Jan 26, 2023 •

edited

Loading

BLoe Jan 26, 2023

twschiller Jan 26, 2023

github-actions bot commented Jan 26, 2023

twschiller commented Jan 26, 2023 •

edited

Loading

fregante commented Jan 27, 2023

#5078: support showing panel as popover #5023

#5078: support showing panel as popover #5023

Conversation

twschiller commented Jan 15, 2023 • edited Loading

What does this PR do?

Known Issues

Demo

Future Work

Checklist

codecov bot commented Jan 25, 2023 • edited Loading

Codecov Report

twschiller Jan 25, 2023 • edited Loading

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 Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fregante commented Jan 26, 2023

BLoe left a 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

twschiller Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 26, 2023

twschiller commented Jan 26, 2023 • edited Loading

fregante commented Jan 27, 2023

twschiller commented Jan 15, 2023 •

edited

Loading

codecov bot commented Jan 25, 2023 •

edited

Loading

twschiller Jan 25, 2023 •

edited

Loading

twschiller Jan 26, 2023 •

edited

Loading

twschiller Jan 26, 2023 •

edited

Loading

twschiller commented Jan 26, 2023 •

edited

Loading