Upgrade DOM bricks to be root aware #4910

twschiller · 2022-12-29T15:29:21Z

What does this PR do?

Implements backward compatible approach described in (Slice 1 and 2): https://www.notion.so/pixiebrix/Update-Non-Root-Aware-Bricks-to-be-Root-Aware-71011e4a523f41afa27dd84f085f7252
- Defines helper method for selecting elements with isRootAware flag
- Marks DOM bricks as root aware
Add test coverage for bricks

Reviewer Tips

Start with rootAwareHelpers
Ensure the bricks have isRootAware() method overriden
Focus on the brick's run methods to double-check this wouldn't change the behavior of any existing bricks

Demo

https://www.loom.com/share/65220eb2e9074077b7503af2d1b59097

Future Work

We'll need to figure out how to upgrade the Set Input Value brick to work directly on an element

Checklist

Add tests
Designate a primary reviewer: @BALEHOK

codecov · 2022-12-29T15:35:44Z

Codecov Report

Merging #4910 (5e1dd2d) into main (a77694e) will increase coverage by 0.77%.
The diff coverage is 86.46%.

❗ Current head 5e1dd2d differs from pull request most recent head 36195d0. Consider uploading reports for the commit 36195d0 to get more accurate results

@@            Coverage Diff             @@
##             main    #4910      +/-   ##
==========================================
+ Coverage   57.97%   58.75%   +0.77%     
==========================================
  Files         960      962       +2     
  Lines       28752    28899     +147     
  Branches     5521     5545      +24     
==========================================
+ Hits        16670    16980     +310     
+ Misses      12082    11919     -163

Impacted Files	Coverage Δ
...s/form/widgets/switchButton/SwitchButtonWidget.tsx	`100.00% <ø> (ø)`
src/hooks/useTimeoutState.ts	`100.00% <ø> (ø)`
src/blocks/effects/tour.ts	`34.09% <27.27%> (+34.09%)`	⬆️
src/blocks/effects/event.ts	`78.26% <70.00%> (+78.26%)`	⬆️
src/blocks/effects/forms.ts	`70.40% <73.68%> (+70.40%)`	⬆️
src/pageEditor/tabs/effect/BlockConfiguration.tsx	`93.65% <83.33%> (-1.18%)`	⬇️
src/blocks/rootModeHelpers.ts	`93.33% <93.33%> (ø)`
src/blocks/effects/attachAutocomplete.ts	`76.66% <100.00%> (+76.66%)`	⬆️
src/blocks/effects/customEvent.ts	`100.00% <100.00%> (+100.00%)`	⬆️
src/blocks/effects/disable.ts	`100.00% <100.00%> (+100.00%)`	⬆️
... and 26 more

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

twschiller · 2022-12-29T15:45:05Z

src/blocks/effects/customEvent.ts

@@ -43,17 +43,17 @@ class CustomEventEffect extends Effect {
    required: ["eventName"],
  };

+  override async isRootAware(): Promise<boolean> {


It wasn't declaring itself as root aware even though it was using the root

src/blocks/effects/disable.ts

vercel · 2022-12-29T16:25:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
pixiebrix-extension	🔄 Building (Inspect)		Jan 2, 2023 at 1:11PM (UTC)

…ware

BALEHOK · 2023-01-02T16:49:10Z

src/blocks/effects/attachAutocomplete.test.ts

@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2022 PixieBrix, Inc.


Please update the year

BALEHOK · 2023-01-02T16:55:46Z

src/blocks/rootModeHelpers.ts

+  if (!isRootAware && isEmpty(selector)) {
+    throw new PropError(
+      "Selector is required",
+      blockId,
+      selectorProp,
+      selector
+    );
+  }
+
+  if (isRootAware) {
+    if (isEmpty(selector)) {
+      return $(root);
+    }
+
+    return $safeFind(selector, root);
+  }
+
+  return $safeFind(selector, document);


NIT: making it based on the isRootAware, check it once and have 2 branches for isRootAware == true and isRootAware == false:

Suggested change

if (!isRootAware && isEmpty(selector)) {

throw new PropError(

"Selector is required",

blockId,

selectorProp,

selector

);

}

if (isRootAware) {

if (isEmpty(selector)) {

return $(root);

}

return $safeFind(selector, root);

}

return $safeFind(selector, document);

if (isRootAware) {

if (isEmpty(selector)) {

return $(root);

}

return $safeFind(selector, root);

}

if (isEmpty(selector)) {

throw new PropError(

"Selector is required",

blockId,

selectorProp,

selector

);

}

return $safeFind(selector, document);

BALEHOK · 2023-01-02T17:05:53Z

src/blocks/rootModeHelpers.ts

+ * Special prop used to upgrade DOM bricks to be root-aware
+ * @since 1.7.16
+ */
+export const IS_ROOT_AWARE_BRICK_PROPS = {


We could freeze the object, just to make sure it's never mutated.

Alternatively we can change the API , exporting a method which injects the isRootAware property into a properties object:

inputSchema: Schema = propertiesToSchema( addRootAwareProperty( { selector: { @@ -45,34 +51,50 @@ export class AttachAutocomplete extends Effect { type: "string", }, }, }), ["options"] );

👍 I'll go ahead an freeze the object

BALEHOK · 2023-01-02T17:14:34Z

src/blocks/effects/attachAutocomplete.ts

+
+    const inputs = $elements
+      .toArray()
+      .filter((x) => !(x instanceof Document) && x.tagName === "INPUT");


NIT: Is !(x instanceof Document) added just to fix a type error?
Looks unnecessary and a little bit confusing.

$elements is a Document or HTMLElement. There's really no way of avoiding putting the check somewhere, and Typescript needs enough confidence that all of the array elements are HTMLElements

BALEHOK · 2023-01-02T17:19:26Z

src/blocks/effects/customEvent.test.ts

+    document.body.innerHTML = `
+      <html>
+        <body>
+           <div>


NIT: there's one extra space in the indent.

BALEHOK · 2023-01-02T17:40:40Z

src/blocks/effects/tour.ts

@@ -130,7 +142,7 @@ export class TourEffect extends Effect {
      }

      const [firstStep] = steps as Step[];
-      if ($safeFind(firstStep.element).length === 0) {
+      if ($safeFind(firstStep.element, $root).length === 0) {


Can we use $safeFindElementsWithRootMode to be consistent?

We can't because the $safeFindElementsWithRootMode assumes in its error message that the selector is a top level prop.

Tours will generally have multiple tour stops, so it's expected that the user is providing a selector for each tour stop

In the future, the user would use the Display Temporary Information brick with location: popover to show a popover on a single element

BALEHOK · 2023-01-02T17:43:11Z

src/blocks/transformers/component/TableReader.ts

+    { selector, orientation = "infer" }: BlockArg,
+    { root }: BlockOptions
+  ): Promise<unknown> {
+    const table = selector ? requireSingleElement(selector, root) : root;


Using root implies that the Reader is root aware, doesn't it? If yes, it should provide the isRootAware property.

The brick was already rootAware with isRootAware() returning true. This PR makes the selector optional. So we don't need to add an isRootAware prop to the brick inputs

BALEHOK · 2023-01-02T19:34:34Z

src/components/fields/schemaFields/RootAwareField.tsx

+
+  // The DOM brick is running in legacy mode.
+  return (
+    <ConnectedFieldTemplate


NIT: This can be replaced by FieldTemplate, it will skip unnecessary re-renders triggered by the Formik updates.

I'd have to replicate the touched/error, etc. logic for it then. I think it's cleaner to use the connected component. We'll see if it has any impact on performance

src/components/fields/schemaFields/SchemaField.test.tsx

src/components/form/widgets/switchButton/SwitchButtonWidget.tsx

…4990)

Update DOM bricks to be root aware

46ecd17

twschiller requested review from BLoe, fregante and BALEHOK December 29, 2022 15:43

twschiller assigned twschiller and BLoe Dec 29, 2022

twschiller marked this pull request as ready for review December 29, 2022 15:43

twschiller commented Dec 29, 2022

View reviewed changes

src/blocks/effects/disable.ts Show resolved Hide resolved

twschiller added this to the 1.7.16 milestone Dec 29, 2022

Page Editor ergonomics

3c112c8

vercel bot deployed to Preview December 29, 2022 16:16 View deployment

twschiller changed the title ~~Update DOM bricks to be root aware~~ Upgrade DOM bricks to be root aware Dec 29, 2022

Improve description

b83b787

vercel bot deployed to Preview December 29, 2022 16:28 View deployment

Add test for SchemaField

7335c85

vercel bot deployed to Preview December 30, 2022 14:16 View deployment

Merge remote-tracking branch 'origin/main' into feature/update-root-a…

fd66463

…ware

vercel bot deployed to Preview January 2, 2023 13:13 View deployment

twschiller assigned BALEHOK and unassigned BLoe Jan 2, 2023

BALEHOK approved these changes Jan 2, 2023

View reviewed changes

twschiller added 2 commits January 2, 2023 15:15

Merge conflict

f3f675b

Address PR comments

5e1dd2d

twschiller force-pushed the feature/update-root-aware branch from 11acf32 to 5e1dd2d Compare January 2, 2023 20:32

vercel bot deployed to Preview January 2, 2023 20:36 View deployment

Fix event test case

36195d0

twschiller enabled auto-merge (squash) January 2, 2023 20:41

vercel bot deployed to Preview January 2, 2023 20:41 View deployment

twschiller merged commit 49b0d92 into main Jan 2, 2023

twschiller deleted the feature/update-root-aware branch January 2, 2023 20:46

twschiller added a commit that referenced this pull request Jan 10, 2023

#4910: make selector optional if root is field

6da8a65

twschiller mentioned this pull request Jan 10, 2023

#4910: make selector for Set Input Value optional if root is field #4990

Merged

2 tasks

twschiller added a commit that referenced this pull request Jan 10, 2023

#4910: make selector for Set Input Value optional if root is field (#…

c8b7b60

…4990)

twschiller mentioned this pull request Jan 26, 2023

#5078: support showing panel as popover #5023

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade DOM bricks to be root aware #4910

Upgrade DOM bricks to be root aware #4910

twschiller commented Dec 29, 2022 •

edited

Loading

codecov bot commented Dec 29, 2022 •

edited

Loading

twschiller Dec 29, 2022

vercel bot commented Dec 29, 2022 •

edited

Loading

BALEHOK Jan 2, 2023

BALEHOK Jan 2, 2023

BALEHOK Jan 2, 2023

twschiller Jan 2, 2023

BALEHOK Jan 2, 2023

twschiller Jan 2, 2023

BALEHOK Jan 2, 2023

BALEHOK Jan 2, 2023

twschiller Jan 2, 2023

BALEHOK Jan 2, 2023

twschiller Jan 2, 2023 •

edited

Loading

BALEHOK Jan 2, 2023

twschiller Jan 2, 2023

Upgrade DOM bricks to be root aware #4910

Upgrade DOM bricks to be root aware #4910

Conversation

twschiller commented Dec 29, 2022 • edited Loading

What does this PR do?

Reviewer Tips

Demo

Future Work

Checklist

codecov bot commented Dec 29, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

vercel bot commented Dec 29, 2022 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twschiller commented Dec 29, 2022 •

edited

Loading

codecov bot commented Dec 29, 2022 •

edited

Loading

vercel bot commented Dec 29, 2022 •

edited

Loading

twschiller Jan 2, 2023 •

edited

Loading