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.

[hatton]

@hatton made 10 comments.
Reviewable status: 2 of 138 files reviewed, 12 unresolved discussions (waiting on JohnThomson).

---

a discussion (no related file):

Previously, hatton (John Hatton) wrote…

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.

Done.

---

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

Previously, hatton (John Hatton) wrote…

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.

Done.

---

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

Previously, hatton (John Hatton) wrote…

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.

Removed all backend involvement in launching the dialog. And we now longer save before opening.

---

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

Previously, hatton (John Hatton) wrote…

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

Done.

---

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

Previously, hatton (John Hatton) wrote…

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

Done.

---

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

Previously, hatton (John Hatton) wrote…

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.

Done.

---

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

Previously, JohnThomson (John Thomson) wrote…

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

i can't find the code this was referring to. Sounds harmless so just marking this Done.

---

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

Previously, hatton (John Hatton) wrote…

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.

Done.

---

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

Previously, hatton (John Hatton) wrote…

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.

Done.

---

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

Previously, JohnThomson (John Thomson) wrote…

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.

The shared structure is already centralized: there is now a single helper that combines ConfigrCustomStringInput with the nested ColorDisplayButton, and the active page color UI calls that helper instead of duplicating that wrapper structure in multiple places. The label lookup is also no longer duplicated in the way the older comment described.

The only small remaining duplication is that the same localized string gets passed as both label and localizedTitle, but that is just reusing one looked-up value for two props, not repeating the lookup or splitting the control composition across multiple functions. So I’d consider the original structural concern addressed.

[hatton]

I think I have resolved all issues now. The PR conversations here needs a fresh start since the code has changed so much. Closing this one, will open another