-
-
Notifications
You must be signed in to change notification settings - Fork 18
BL-16069 Disable tools during Change Layout mode #7828
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,7 @@ | ||||||||||||||||||||||||||||||
| import * as React from "react"; | ||||||||||||||||||||||||||||||
| import { css } from "@emotion/react"; | ||||||||||||||||||||||||||||||
| import { get } from "../utils/bloomApi"; | ||||||||||||||||||||||||||||||
| import { setToolboxEnabledHandler } from "../bookEdit/workspaceRoot"; | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| interface IEditFrameSources { | ||||||||||||||||||||||||||||||
| pageListSrc: string; | ||||||||||||||||||||||||||||||
|
|
@@ -22,6 +23,7 @@ export const EditTabPane: React.FunctionComponent<{ active: boolean }> = ( | |||||||||||||||||||||||||||||
| defaultEditFrameSources, | ||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||
| const [toolboxIsShowing, setToolboxIsShowing] = React.useState(true); | ||||||||||||||||||||||||||||||
| const [toolboxIsEnabled, setToolboxIsEnabled] = React.useState(true); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| // Request edit-frame URLs from C# when edit mode becomes active so app.tsx controls iframe rendering. | ||||||||||||||||||||||||||||||
| React.useEffect(() => { | ||||||||||||||||||||||||||||||
|
|
@@ -39,6 +41,20 @@ export const EditTabPane: React.FunctionComponent<{ active: boolean }> = ( | |||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
| }, [props.active]); | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| React.useEffect(() => { | ||||||||||||||||||||||||||||||
| // We will use this to disable and hide the toolbox when in Change Layout mode (BL-16069) | ||||||||||||||||||||||||||||||
| setToolboxEnabledHandler((enabled) => { | ||||||||||||||||||||||||||||||
| setToolboxIsEnabled(enabled); | ||||||||||||||||||||||||||||||
| if (!enabled) { | ||||||||||||||||||||||||||||||
| setToolboxIsShowing(false); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||
|
Comment on lines
+46
to
+51
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Toolbox showing state not restored when toolbox is re-enabled after exiting Change Layout mode When the toolbox is disabled (entering Change Layout mode), the handler at The
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return () => { | ||||||||||||||||||||||||||||||
| setToolboxEnabledHandler(undefined); | ||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||
| }, []); | ||||||||||||||||||||||||||||||
|
Comment on lines
+44
to
+56
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🚩 Handler registration uses stale closure for reading state but this doesn't affect current code The handler registered in the Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| return ( | ||||||||||||||||||||||||||||||
| <div | ||||||||||||||||||||||||||||||
| css={css` | ||||||||||||||||||||||||||||||
|
|
@@ -88,6 +104,15 @@ export const EditTabPane: React.FunctionComponent<{ active: boolean }> = ( | |||||||||||||||||||||||||||||
| top: 3px; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| .pure-toggle-label.toolbox-disabled { | ||||||||||||||||||||||||||||||
| opacity: 0.45; | ||||||||||||||||||||||||||||||
| cursor: default; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| .pure-toggle-label.toolbox-disabled .pure-toggle-icon { | ||||||||||||||||||||||||||||||
| filter: grayscale(1); | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| .pure-drawer { | ||||||||||||||||||||||||||||||
| position: absolute; | ||||||||||||||||||||||||||||||
| top: 0; | ||||||||||||||||||||||||||||||
|
|
@@ -123,14 +148,21 @@ export const EditTabPane: React.FunctionComponent<{ active: boolean }> = ( | |||||||||||||||||||||||||||||
| id="pure-toggle-right" | ||||||||||||||||||||||||||||||
| data-toggle="right" | ||||||||||||||||||||||||||||||
| checked={toolboxIsShowing} | ||||||||||||||||||||||||||||||
| disabled={!toolboxIsEnabled} | ||||||||||||||||||||||||||||||
| onChange={(event) => { | ||||||||||||||||||||||||||||||
| if (!toolboxIsEnabled) { | ||||||||||||||||||||||||||||||
| return; | ||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||
| setToolboxIsShowing(event.currentTarget.checked); | ||||||||||||||||||||||||||||||
| }} | ||||||||||||||||||||||||||||||
| /> | ||||||||||||||||||||||||||||||
| <label | ||||||||||||||||||||||||||||||
| className="pure-toggle-label" | ||||||||||||||||||||||||||||||
| className={`pure-toggle-label${ | ||||||||||||||||||||||||||||||
| toolboxIsEnabled ? "" : " toolbox-disabled" | ||||||||||||||||||||||||||||||
| }`} | ||||||||||||||||||||||||||||||
| htmlFor="pure-toggle-right" | ||||||||||||||||||||||||||||||
| data-toggle-label="right" | ||||||||||||||||||||||||||||||
| aria-disabled={!toolboxIsEnabled} | ||||||||||||||||||||||||||||||
| > | ||||||||||||||||||||||||||||||
| <span className="pure-toggle-icon" /> | ||||||||||||||||||||||||||||||
| </label> | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -17,6 +17,7 @@ export interface IWorkspaceExports { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| options: JQueryUI.DialogOptions, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): JQuery; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| closeDialog(id: string): void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setToolboxEnabled(enabled: boolean): void; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toolboxIsShowing(): boolean; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| doWhenToolboxLoaded( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| task: (toolboxFrameExports: IToolboxFrameExports) => unknown, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -188,6 +189,22 @@ export function toolboxIsShowing() { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| return checkbox ? checkbox.checked : true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // The toolbox should be disabled whenever we are in Change Layout mode (BL-16069) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let toolboxEnabled = true; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let toolboxEnabledHandler: ((enabled: boolean) => void) | undefined; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function setToolboxEnabledHandler( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| handler: ((enabled: boolean) => void) | undefined, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toolboxEnabledHandler = handler; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toolboxEnabledHandler?.(toolboxEnabled); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function setToolboxEnabled(enabled: boolean): void { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toolboxEnabled = enabled; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toolboxEnabledHandler?.(enabled); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+196
to
+206
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔴 Missing comments on new exported functions setToolboxEnabledHandler and setToolboxEnabled Per AGENTS.md: "All public methods should have a comment. So should most private ones!" Both
Suggested change
Was this helpful? React with 👍 or 👎 to provide feedback. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Do this task when the toolbox is loaded. If it isn't already, we set a timeout and do it when we can. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // (The value passed to the task function will be the value from getToolboxBundleExports(). Unfortunately we | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // haven't yet managed to declare a type for that, so I can't easily specify it here.) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -359,6 +376,7 @@ interface WorkspaceBundleApi { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| switchContentPage: typeof switchContentPage; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| showDialog: typeof showDialog; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| closeDialog: typeof closeDialog; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setToolboxEnabled: typeof setToolboxEnabled; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toolboxIsShowing: typeof toolboxIsShowing; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| doWhenToolboxLoaded: typeof doWhenToolboxLoaded; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| canUndo: typeof canUndo; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -399,6 +417,7 @@ window.workspaceBundle = { | |||||||||||||||||||||||||||||||||||||||||||||||||||||
| switchContentPage, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| showDialog, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| closeDialog, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| setToolboxEnabled, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| toolboxIsShowing, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| doWhenToolboxLoaded, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| canUndo, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: Toolbox visibility is not restored when re-enabled. When entering Change Layout mode,
toolboxIsShowingis set tofalse, but whenenabledbecomestrueagain the handler doesn't restore it. This means after exiting Change Layout mode the toolbox stays hidden even though the toggle is re-enabled.Consider saving the previous
toolboxIsShowingvalue and restoring it when re-enabled.Prompt for AI agents