BL-15642 Intro Page Settings

[hatton]

This change is 

---

[JohnThomson]

@JohnThomson reviewed 15 files and all commit messages, and made 13 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @hatton).

---

a discussion (no related file):
Some of my comments are high-level structural things, and this may not be the place to address them. But you've asked me to look for places code quality and architecture could be improved.
Most of the rest are not of great importance, but you should definitely consider whether you want this button on Game pages and if so whether it successfully coexists with the Game controls.

---

src/content/appearanceThemes/appearance-theme-rounded-border-ebook.css line 31 at r2 (raw file):

[class*="Device"].numberedPage:not(.bloom-interactive-page) {
    --pageNumber-extra-height: 0mm !important; /* we put the page number on top of the image so we don't need a margin boost */
    --pageNumber-background-color: #ffffff; /* I'm not clear why this is white, but all I did in this change is to move it so that it can be overridden by page settings */

Possibly because it's otherwise transparent, and this theme often puts it on top of an image, where it might not be legible without a background color?

---

src/BloomBrowserUI/react_components/color-picking/colorPickerDialog.tsx line 289 at r2 (raw file):

    // The MUI backdrop is rendered outside the dialog tree, so we use a body class
    // to suppress it while the color picker is open.

Seems to me to need explanation: why do we want to suppress the backdrop for this dialog?

---

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 31 at r2 (raw file):

import tinycolor from "tinycolor2";

let isOpenAlready = false;

Just isOpen? or isPageSettingsDialogOpen? The code here seems to entirely manage this variable, whereas 'already' suggests that something else might have caused it to be open before this code got started.

---

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 41 at r2 (raw file):

};

const getCurrentPageElement = (): HTMLElement => {

There's probably an existing function (quite likely more than one) that you could import to do this.

---

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 73 at r2 (raw file):

const getComputedStyleForPage = (page: HTMLElement): CSSStyleDeclaration => {
    const view = page.ownerDocument.defaultView;

Deserves a comment. Why would a page not have an ownerDocument? Why would it give a more useful answer for getComputedStyle()?

---

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 162 at r2 (raw file):

            "--pageNumber-background-color",
        ),
    );

Should it have the same special cases as getCurrentPageBackgroundColor? Or better, just call that? If not, it might be helpful to explain why.

---

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 390 at r2 (raw file):

                                    applyPageSettings(settings);
                                    return;
                                }

It feels wrong that we should have to handle two different kinds of argument in onChange. Maybe there's a good reason Configr is implemented that way, but I think it would be better to make it return one or the other. Maybe ConfigrPane could take a that tells it what "s" should be and could parse the string itself if need be? Or at least there could be a wrapper like that so every client doesn't have to do what you are doing here.

---

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 422 at r2 (raw file):

                                        }
                                        disabled={false}
                                    />

The structure feels wrong here. The knowledge that each control is a ConfigrCustomStringInput wrapping a ColorDisplayButton is both duplicated three times and split between this code and the individul functions, and the knowledge of how to get each label is used twice each. Consider making a single function that combines a ConfigrCustomStringInput and a nested ColorDisplayButton, with just enough props to implement the three variations, including looking up the label. You could then either call that three times here, or define the three methods each to call it with appropriate arguments, and here just call the three functions.

---

src/BloomBrowserUI/bookEdit/js/origami.ts line 360 at r2 (raw file):

${getPageSettingsButtonHtml()}\
</div>`,
        );

Do you want this button in Game pages, which is where the previously empty version is used? If so this probably needs attention, since I think the Games code uses this whole element to render the Start/Correct/Wrong/Play control.
(Maybe it's time to re-implement the origami on/off switch in React, and then we could have a single react control responsible for the whole decision about what to show in this space?)

---

src/BloomBrowserUI/bookEdit/js/origami.ts line 391 at r2 (raw file):

function pageSettingsButtonClickHandler(e: Event) {
    e.preventDefault();
    post("editView/showPageSettingsDialog");

It looks like we go through C# here just so we can save the page first. Is that essential? I would think it would be better to save afterwards, if this action even calls for a Save, so the page thumbnail can update, especially if the background color changed.

---

src/BloomExe/Edit/EditingView.cs line 1928 at r2 (raw file):

        }

        private void _pageSettingsButton_Click(object sender, EventArgs e)

not used? Maybe there was a C# button in early testing?

[hatton]

@hatton made 1 comment.
Reviewable status: 3 of 76 files reviewed, 20 unresolved discussions (waiting on JohnThomson).

---

src/BloomBrowserUI/react_components/color-picking/colorPickerDialog.tsx line 289 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Seems to me to need explanation: why do we want to suppress the backdrop for this dialog?

  [codex] The purpose is to avoid interference while picking colors/eyedropper interactions and to prevent layered backdrop behavior across nested dialogs. Existing comments already explain this, and the CSS note also states the intent (minimal interference). I did not change behavior here.

- Action: No code change (documentation intent already present).

[hatton]

@hatton made 1 comment.
Reviewable status: 3 of 76 files reviewed, 20 unresolved discussions (waiting on JohnThomson).

---

a discussion (no related file):

Previously, JohnThomson (John Thomson) wrote…

Some of my comments are high-level structural things, and this may not be the place to address them. But you've asked me to look for places code quality and architecture could be improved.
Most of the rest are not of great importance, but you should definitely consider whether you want this button on Game pages and if so whether it successfully coexists with the Game controls.

Will do, TODO

[hatton]

Previously, hatton (John Hatton) wrote…

Will do, TODO

Will do, TODO [codex] I kept the button on game pages (as discussed) and also expanded visibility to non-custom pages. The game control container flow remains unchanged (GameTool still appends into .above-page-control-container). This should coexist, but a quick manual check on a game page is still recommended.

[hatton]

src/content/appearanceThemes/appearance-theme-rounded-border-ebook.css line 31 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Possibly because it's otherwise transparent, and this theme often puts it on top of an image, where it might not be legible without a background color?

Will do, TODO [codex] Agreed. I treated this as intentional legibility behavior and aligned migration behavior accordingly by restoring white for efl-zeromargin1.

[hatton]

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 41 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

There's probably an existing function (quite likely more than one) that you could import to do this.

Will do, TODO [codex] Agreed in principle. In the current branch, this helper now lives in PageSettingsConfigrPages.tsx and is reused by the integrated dialog flow. I did not find an additional shared helper that would reduce duplication further without adding cross-module coupling.

[hatton]

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 390 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It feels wrong that we should have to handle two different kinds of argument in onChange. Maybe there's a good reason Configr is implemented that way, but I think it would be better to make it return one or the other. Maybe ConfigrPane could take a that tells it what "s" should be and could parse the string itself if need be? Or at least there could be a wrapper like that so every client doesn't have to do what you are doing here.

Will do, TODO [codex] Good catch. In the vendored @sillsdev/config-r currently used by this branch, ConfigrPane/ContentPane onChange already returns a plain object (ConfigrValues), not a JSON string. I removed the now-unnecessary local string/object normalization paths from our dialogs so we no longer branch per client.

[hatton]

src/BloomBrowserUI/bookEdit/js/origami.ts line 360 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Do you want this button in Game pages, which is where the previously empty version is used? If so this probably needs attention, since I think the Games code uses this whole element to render the Start/Correct/Wrong/Play control.
(Maybe it's time to re-implement the origami on/off switch in React, and then we could have a single react control responsible for the whole decision about what to show in this space?)

Will do, TODO [codex] I kept the button on game pages and preserved the container behavior used by GameTool. The implementation still allows game controls to append to .above-page-control-container. Manual verification on a game page is still advisable.

[hatton]

src/BloomExe/Edit/EditingView.cs line 1928 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

not used? Maybe there was a C# button in early testing?

Will do, TODO [codex] Agreed. I removed the unused _pageSettingsButton_Click method.

[hatton]

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 31 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Just isOpen? or isPageSettingsDialogOpen? The code here seems to entirely manage this variable, whereas 'already' suggests that something else might have caused it to be open before this code got started.

Will do, TODO [codex] The referenced standalone PageSettingsDialog.tsx was replaced by the integrated Book/Page settings dialog implementation. The current code path uses BookAndPageSettingsDialog.tsx with existing naming patterns there. I did not make a naming-only change in this pass.

[hatton]

src/BloomBrowserUI/bookEdit/pageSettings/PageSettingsDialog.tsx line 162 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

Should it have the same special cases as getCurrentPageBackgroundColor? Or better, just call that? If not, it might be helpful to explain why.

Will do, TODO [codex] I left this as-is. The page-number background intentionally has different semantics from page background and defaults to unset/transparent unless explicitly themed or set. Reusing page-background logic here would risk conflating those behaviors.

[hatton]

src/BloomBrowserUI/bookEdit/js/origami.ts line 391 at r2 (raw file):

Previously, JohnThomson (John Thomson) wrote…

It looks like we go through C# here just so we can save the page first. Is that essential? I would think it would be better to save afterwards, if this action even calls for a Save, so the page thumbnail can update, especially if the background color changed.

Will do, TODO [codex] I kept save-before-open. This was a conscious decision for this branch to ensure consistent state entering settings and avoid introducing workflow regression late in the cycle.

[devin-ai-integration]

Devin Review found 1 new potential issue.

View 16 additional findings in Devin Review.

[devin-ai-integration]

🟡 Legacy theme check hardcoded instead of using isLegacyThemeName utility

The old BookSettingsDialog used isLegacyThemeName(theme) from appearanceThemeUtils to determine if the theme was legacy. The new BookAndPageSettingsDialog hardcodes a direct string comparison liveAppearance?.cssThemeName === "legacy-5-6" at line 260, and props.theme === "legacy-5-6" in BookSettingsConfigrPages.tsx at line 321. If isLegacyThemeName at src/BloomBrowserUI/bookEdit/bookAndPageSettings/appearanceThemeUtils.ts handles additional legacy theme name variants (or is updated to do so in the future), these hardcoded checks will be out of sync, potentially allowing legacy-incompatible controls to be enabled when they shouldn't be.

Prompt for agents

In src/BloomBrowserUI/bookEdit/bookAndPageSettings/BookAndPageSettingsDialog.tsx line 260, replace the hardcoded string comparison `liveAppearance?.cssThemeName === "legacy-5-6"` with `isLegacyThemeName(liveAppearance?.cssThemeName)`, importing `isLegacyThemeName` from `./appearanceThemeUtils`. Similarly, in src/BloomBrowserUI/bookEdit/bookAndPageSettings/BookSettingsConfigrPages.tsx, replace `props.theme === "legacy-5-6"` at line 321 and `props.theme !== "legacy-5-6"` at lines 331 and 388 with `isLegacyThemeName(props.theme)` / `!isLegacyThemeName(props.theme)`. This keeps the logic consistent with how BookInfoIndicator.tsx (which was also updated in this PR) uses isLegacyThemeName.

---

Was this helpful? React with 👍 or 👎 to provide feedback.