#3566: add support for switching active sidebar panel via brick; automatically switch when using Page Editor #3567

twschiller · 2022-06-05T17:43:20Z

What does this PR do?

Closes Add support for switching the active sidebar panel via brick #3566
Add forcePanel and panelHeading options to the Show Sidebar brick
By default, the Show Sidebar brick will activate a panel from the current blueprint
User can also specify a panelHeading to match to activate
When rendering a panel via the Page Editor, switch over to that panel
When selecting an extension in the Page Editor, switch over to that panel
Keep a stable panel order in the sidebar (just based on sorting extensions UUIDs; non-intuitive, but it's stable)

Considerations/Discussion

In the future we might want to make panelHeading support a regex. Technically, panel headers can vary dynamically (templating over the reader for the panel). However, that's not currently possible with sidebar panels made with the Page Editor

Reviewer Notes

Take a look at sidebarController first. It's the module that handles the sidebar in the content script
Take a look at sidebarSlice next. It's the Redux slice for the Sidebar App
Take a look at SidebarApp.tsx. It has the listener bridge between the messenger/contentScript and the redux state

Tasks Remaining

Fix race condition in updateDynamicElement and show sidebar brick when trying to show a panel on initial load

Out of scope

Reduce/eliminate panel flickering

Checklist

Add tests
Designate a primary reviewer: @BALEHOK

codecov-commenter · 2022-06-05T17:47:13Z

Codecov Report

Merging #3567 (0afbab4) into main (76ab769) will decrease coverage by 0.10%.
The diff coverage is 16.07%.

@@            Coverage Diff             @@
##             main    #3567      +/-   ##
==========================================
- Coverage   45.27%   45.16%   -0.11%     
==========================================
  Files         790      791       +1     
  Lines       23215    23293      +78     
  Branches     4819     4843      +24     
==========================================
+ Hits        10510    10521      +11     
- Misses      11823    11878      +55     
- Partials      882      894      +12

Impacted Files	Coverage Δ
src/blocks/effects/sidebar.ts	`0.00% <0.00%> (ø)`
src/contentScript/lifecycle.ts	`0.00% <ø> (ø)`
src/contentScript/messenger/registration.ts	`0.00% <ø> (ø)`
src/contentScript/nativeEditor/dynamic.ts	`0.00% <0.00%> (ø)`
src/core.ts	`100.00% <ø> (ø)`
src/pageEditor/sidebar/InstalledEntry.tsx	`69.23% <0.00%> (-2.77%)`	⬇️
src/sidebar/SidebarApp.tsx	`0.00% <0.00%> (ø)`
src/sidebar/messenger/registration.ts	`0.00% <ø> (ø)`
src/sidebar/protocol.tsx	`0.00% <0.00%> (ø)`
src/sidebar/sidebarSlice.ts	`0.00% <0.00%> (ø)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 76ab769...0afbab4. Read the comment docs.

…idebar-panel

twschiller · 2022-06-05T19:45:30Z

src/utils/domUtils.ts

+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+export function getHTMLElement(): JQuery {


Moved to a utility file because it's not specific to the sidebar

BALEHOK · 2022-06-06T14:10:46Z

src/contentScript/sidebarController.tsx

@@ -133,6 +130,8 @@ export function showSidebar(callbacks = extensionCallbacks): string {

  let nonce = container?.dataset?.nonce;

+  const isShowing = Boolean(nonce);
+
  if (!nonce) {


NIT:

Suggested change

if (!nonce) {

if (!isShowing) {

BALEHOK · 2022-06-06T14:16:10Z

src/contentScript/sidebarController.tsx

@@ -151,15 +150,57 @@ export function showSidebar(callbacks = extensionCallbacks): string {
    }
  }

+  if (!isEmpty(activateOptions)) {


Why activateOptions can't be null by default? Then the check will be simpler:

Suggested change

if (!isEmpty(activateOptions)) {

if (activateOptions != null) {

Why activateOptions can't be null by default? Then the check will be simpler

I think keeping track of null vs. non-null is generally more error-prone. If there's a valid "empty"/default value I like to use that instead

BALEHOK · 2022-06-06T14:16:33Z

src/contentScript/sidebarController.tsx

+        );
+      });
+  }
+
  // TODO: Drop `nonce` if not used by the caller


is this TODO still relevant?

is this TODO still relevant?

Not sure, didn't want to spend the time double-checking all the call sites. It's not hurting anything to leave the value in

BALEHOK · 2022-06-06T14:36:21Z

src/blocks/transformers/ephemeralForm/formTransformer.ts

@@ -29,7 +29,7 @@ import {
  hideSidebarForm,
  PANEL_HIDING_EVENT,
  showSidebarForm,
-} from "@/contentScript/sidebar";
+} from "@/contentScript/sidebarController";


#3566: add support for switching active sidebar panel via brick

fae92b2

twschiller changed the title ~~#3566: add support for switching active sidebar panel via brick~~ #3566: [WIP] add support for switching active sidebar panel via brick Jun 5, 2022

twschiller marked this pull request as draft June 5, 2022 17:43

twschiller added 3 commits June 5, 2022 13:56

Merge remote-tracking branch 'origin/main' into feature/3566-active-s…

037bc90

…idebar-panel

#3566: switch to panel being edited

99c46de

#3566: improve property name

9740ae0

twschiller changed the title ~~#3566: [WIP] add support for switching active sidebar panel via brick~~ #3566: add support for switching active sidebar panel via brick Jun 5, 2022

twschiller added this to the 1.6.5 milestone Jun 5, 2022

twschiller requested a review from BLoe June 5, 2022 18:52

twschiller self-assigned this Jun 5, 2022

#3566: fix race condition in opening sidebar to a given tab

2323dcd

twschiller commented Jun 5, 2022

View reviewed changes

twschiller marked this pull request as ready for review June 5, 2022 19:47

twschiller changed the title ~~#3566: add support for switching active sidebar panel via brick~~ #3566: add support for switching active sidebar panel via brick; automatically switch when using Page Editor Jun 5, 2022

twschiller mentioned this pull request Jun 6, 2022

#3570: eliminate flickering in sidebar panels #3571

Merged

2 tasks

twschiller requested review from BALEHOK and BLoe and removed request for BLoe June 6, 2022 00:05

#3566: fix race condition on initial sidebar open

0afbab4

BALEHOK approved these changes Jun 6, 2022

View reviewed changes

#3566: improve condition

09521f1

twschiller enabled auto-merge (squash) June 6, 2022 14:49

twschiller merged commit fc8e3e3 into main Jun 6, 2022

twschiller deleted the feature/3566-active-sidebar-panel branch June 6, 2022 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3566: add support for switching active sidebar panel via brick; automatically switch when using Page Editor #3567

#3566: add support for switching active sidebar panel via brick; automatically switch when using Page Editor #3567

twschiller commented Jun 5, 2022 •

edited

codecov-commenter commented Jun 5, 2022 •

edited

twschiller Jun 5, 2022

BALEHOK Jun 6, 2022

BALEHOK Jun 6, 2022

twschiller Jun 6, 2022

BALEHOK Jun 6, 2022

twschiller Jun 6, 2022 •

edited

BALEHOK Jun 6, 2022

	if (!isEmpty(activateOptions)) {
	if (activateOptions != null) {

#3566: add support for switching active sidebar panel via brick; automatically switch when using Page Editor #3567

#3566: add support for switching active sidebar panel via brick; automatically switch when using Page Editor #3567

Conversation

twschiller commented Jun 5, 2022 • edited

What does this PR do?

Considerations/Discussion

Reviewer Notes

Tasks Remaining

Out of scope

Checklist

codecov-commenter commented Jun 5, 2022 • edited

Codecov Report

twschiller Jun 5, 2022

Choose a reason for hiding this comment

BALEHOK Jun 6, 2022

Choose a reason for hiding this comment

BALEHOK Jun 6, 2022

Choose a reason for hiding this comment

twschiller Jun 6, 2022

Choose a reason for hiding this comment

BALEHOK Jun 6, 2022

Choose a reason for hiding this comment

twschiller Jun 6, 2022 • edited

Choose a reason for hiding this comment

BALEHOK Jun 6, 2022

Choose a reason for hiding this comment

twschiller commented Jun 5, 2022 •

edited

codecov-commenter commented Jun 5, 2022 •

edited

twschiller Jun 6, 2022 •

edited