Adds the cover layout control and allows a 'custom' cover (BL-15902)#7697
Adds the cover layout control and allows a 'custom' cover (BL-15902)#7697JohnThomson wants to merge 1 commit intomasterfrom
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
JohnThomson
force-pushed
the
customCover3
branch
from
fa54751 to
17a310e
Compare
| @@ -1043,10 +1070,14 @@ function isTextSelected(): boolean { | |||
|
|
|||
| let reportedTextSelected = isTextSelected(); | |||
|
|
|||
There was a problem hiding this comment.
🚩 hideInactiveMarginBox relies on module-level mutable state
The inactiveMarginBox variable at src/BloomBrowserUI/bookEdit/js/bloomEditing.ts:1072 is a module-level mutable variable that tracks a DOM element removed from the document. It is set during hideInactiveMarginBox() (called at bootstrap, line 1082), and restored during restoreInactiveMarginBox() (called during save, line 1310). This pattern is fragile: if hideInactiveMarginBox is called twice without an intervening restore (e.g., due to an unexpected page reload), the first removed element would be lost. The code at convertCoverPageToCustom (customCover.tsx:86) calls restoreInactiveMarginBox() and later hideInactiveMarginBox() (line 272), and the save path (requestPageContentInternal at line 1310) calls restore. The sequence seems carefully managed, but any deviation from the expected call order could silently lose a margin box. The restoreInactiveMarginBox function does clear the variable after restoring (line 1381-1382), which helps prevent double-insertion.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Yes, there's some fragility here. I don't know how to improve it.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
JohnThomson
force-pushed
the
customCover3
branch
from
17a310e to
29b7924
Compare
JohnThomson
changed the title
src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx
Outdated
Show resolved
Hide resolved
src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx
Outdated
Show resolved
Hide resolved
| for (const fieldType of fieldTypeData) { | ||
| subMenu.push({ | ||
| l10nId: null, | ||
| english: fieldType.label, | ||
| onClick: () => { | ||
| for (const className of fieldType.classes) { | ||
| tg.classList.add(className); | ||
| } | ||
| const editables = Array.from( | ||
| tg.getElementsByClassName("bloom-editable"), | ||
| ); | ||
| if (fieldType.readOnly) { | ||
| const readOnlyDiv = document.createElement("div"); | ||
| readOnlyDiv.setAttribute( | ||
| "data-derived", | ||
| fieldType.dataDerived, | ||
| ); | ||
| if (fieldType.hint) { | ||
| readOnlyDiv.setAttribute("data-hint", fieldType.hint); | ||
| } | ||
| if (fieldType.functionOnHintClick) { | ||
| readOnlyDiv.setAttribute( | ||
| "data-functiononhintclick", | ||
| fieldType.functionOnHintClick, | ||
| ); | ||
| } | ||
| readOnlyDiv.classList.add(...fieldType.classes); | ||
| tg.parentElement!.insertBefore(readOnlyDiv, tg); | ||
| tg.remove(); | ||
| // Reload the page to get the derived content loaded. | ||
| post("common/saveChangesAndRethinkPageEvent", () => {}); | ||
| } else { | ||
| tg.classList.add(...fieldType.classes); | ||
| for (const editable of editables) { | ||
| editable.setAttribute("data-book", fieldType.dataBook); | ||
| if ( | ||
| editable.classList.contains( | ||
| "bloom-visibility-code-on", | ||
| ) | ||
| ) { | ||
| getString( | ||
| `editView/getDataBookValue?lang=${editable.getAttribute("lang")}&dataBook=${fieldType.dataBook}`, | ||
| (content) => { | ||
| // content comes from a source that looked empty, we don't want to overwrite something the user may | ||
| // already have typed here. | ||
| // But it may well have something in it, because we usually have an empty paragraph to start with. | ||
| // To test whether it looks empty, we put the text into a newly created element and then | ||
| // see whether it's textContent is empty. | ||
| // The logic of overwriting something which the user has typed here is that if we keep what's here, | ||
| // then the user may never know that there was already something in that field. But if we overwrite, then | ||
| // the user can always correct it back to what he just typed. | ||
| const temp = document.createElement("div"); | ||
| temp.innerHTML = content || ""; | ||
| if (temp.textContent.trim() !== "") | ||
| editable.innerHTML = content; | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| setMenuOpen(false); | ||
| }, | ||
| icon: activeType === fieldType.dataBook && ( | ||
| <CheckIcon css={getMenuIconCss()} /> | ||
| ), | ||
| }); |
There was a problem hiding this comment.
🚩 Field type menu does not clean up old classes/attributes when switching between field types
In makeFieldTypeMenuItem at CanvasElementContextControls.tsx:1187-1241, when a user selects a new field type, classes from fieldType.classes are added to the translation group and data-book is set on editables. However, if the user had previously set a different field type, the old field type's classes and data-book value are not removed before applying the new one. For example, switching from 'Book Title' (adds class bookTitle) to 'Cover Credits' (adds class smallCoverCredits) would leave the bookTitle class on the TG. This could cause confusing styling or data-binding behavior. The 'None' option only removes data-book from editables but also doesn't clean up TG classes.
Was this helpful? React with 👍 or 👎 to provide feedback.
JohnThomson
force-pushed
the
customCover3
branch
from
29b7924 to
a6ed7bb
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 3 comments and resolved 2 discussions.
Reviewable status: 0 of 30 files reviewed, 1 unresolved discussion.
src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx
Outdated
Show resolved
Hide resolved
| for (const fieldType of fieldTypeData) { | ||
| subMenu.push({ | ||
| l10nId: null, | ||
| english: fieldType.label, | ||
| onClick: () => { | ||
| for (const className of fieldType.classes) { | ||
| tg.classList.add(className); | ||
| } | ||
| const editables = Array.from( | ||
| tg.getElementsByClassName("bloom-editable"), | ||
| ); | ||
| if (fieldType.readOnly) { | ||
| const readOnlyDiv = document.createElement("div"); | ||
| readOnlyDiv.setAttribute( | ||
| "data-derived", | ||
| fieldType.dataDerived, | ||
| ); | ||
| if (fieldType.hint) { | ||
| readOnlyDiv.setAttribute("data-hint", fieldType.hint); | ||
| } | ||
| if (fieldType.functionOnHintClick) { | ||
| readOnlyDiv.setAttribute( | ||
| "data-functiononhintclick", | ||
| fieldType.functionOnHintClick, | ||
| ); | ||
| } | ||
| readOnlyDiv.classList.add(...fieldType.classes); | ||
| tg.parentElement!.insertBefore(readOnlyDiv, tg); | ||
| tg.remove(); | ||
| // Reload the page to get the derived content loaded. | ||
| post("common/saveChangesAndRethinkPageEvent", () => {}); | ||
| } else { | ||
| tg.classList.add(...fieldType.classes); | ||
| for (const editable of editables) { | ||
| editable.setAttribute("data-book", fieldType.dataBook); | ||
| if ( | ||
| editable.classList.contains( | ||
| "bloom-visibility-code-on", | ||
| ) | ||
| ) { | ||
| getString( | ||
| `editView/getDataBookValue?lang=${editable.getAttribute("lang")}&dataBook=${fieldType.dataBook}`, | ||
| (content) => { | ||
| // content comes from a source that looked empty, we don't want to overwrite something the user may | ||
| // already have typed here. | ||
| // But it may well have something in it, because we usually have an empty paragraph to start with. | ||
| // To test whether it looks empty, we put the text into a newly created element and then | ||
| // see whether it's textContent is empty. | ||
| // The logic of overwriting something which the user has typed here is that if we keep what's here, | ||
| // then the user may never know that there was already something in that field. But if we overwrite, then | ||
| // the user can always correct it back to what he just typed. | ||
| const temp = document.createElement("div"); | ||
| temp.innerHTML = content || ""; | ||
| if (temp.textContent.trim() !== "") | ||
| editable.innerHTML = content; | ||
| }, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| setMenuOpen(false); | ||
| }, | ||
| icon: activeType === fieldType.dataBook && ( | ||
| <CheckIcon css={getMenuIconCss()} /> | ||
| ), | ||
| }); |
src/BloomBrowserUI/bookEdit/js/CanvasElementContextControls.tsx
Outdated
Show resolved
Hide resolved
|
|
||
| const fieldTypeData: Array<{ | ||
| dataBook: string; | ||
| dataDerived: string; | ||
| label: string; | ||
| readOnly: boolean; | ||
| editableClasses: string[]; | ||
| classes: string[]; | ||
| hint?: string; | ||
| functionOnHintClick?: string; | ||
| }> = [ | ||
| { | ||
| dataBook: "bookTitle", | ||
| dataDerived: "", | ||
| label: "Book Title", | ||
| readOnly: false, | ||
| editableClasses: ["Title-On-Cover-style", "bloom-padForOverflow"], | ||
| classes: ["bookTitle"], | ||
| }, | ||
| { | ||
| dataBook: "smallCoverCredits", | ||
| dataDerived: "", | ||
| label: "Cover Credits", | ||
| readOnly: false, | ||
| editableClasses: ["smallCoverCredits"], | ||
| classes: [], | ||
| }, | ||
| { | ||
| dataBook: "", | ||
| dataDerived: "languagesOfBook", | ||
| label: "Languages", | ||
| readOnly: true, | ||
| editableClasses: [], | ||
| classes: ["coverBottomLangName", "Cover-Default-style"], | ||
| }, | ||
| { | ||
| dataBook: "", | ||
| dataDerived: "topic", | ||
| label: "Topic", | ||
| readOnly: true, | ||
| editableClasses: [], | ||
| classes: [ | ||
| "coverBottomBookTopic", | ||
| "bloom-userCannotModifyStyles", | ||
| "bloom-alwaysShowBubble", | ||
| "Cover-Default-style", | ||
| ], | ||
| hint: "Click to choose topic", | ||
| functionOnHintClick: "showTopicChooser", | ||
| }, | ||
| ]; |
There was a problem hiding this comment.
🚩 fieldTypeData.editableClasses is defined but never used
The editableClasses field is defined in the fieldTypeData type (line 1101) and populated with values like ["Title-On-Cover-style", "bloom-padForOverflow"] for bookTitle (line 1112) and ["smallCoverCredits"] for Cover Credits (line 1120). However, no code in makeFieldTypeMenuItem or elsewhere reads this field. It appears this was intended to be applied to the bloom-editable elements when switching field types, but was never implemented. The absence means that when changing a field's type to "Book Title", the editables don't get the expected Title-On-Cover-style class.
Was this helpful? React with 👍 or 👎 to provide feedback.
JohnThomson
force-pushed
the
customCover3
branch
from
a6ed7bb to
c3f8b03
Compare
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 3 comments.
Reviewable status: 0 of 30 files reviewed, 4 unresolved discussions.
|
|
||
| const fieldTypeData: Array<{ | ||
| dataBook: string; | ||
| dataDerived: string; | ||
| label: string; | ||
| readOnly: boolean; | ||
| editableClasses: string[]; | ||
| classes: string[]; | ||
| hint?: string; | ||
| functionOnHintClick?: string; | ||
| }> = [ | ||
| { | ||
| dataBook: "bookTitle", | ||
| dataDerived: "", | ||
| label: "Book Title", | ||
| readOnly: false, | ||
| editableClasses: ["Title-On-Cover-style", "bloom-padForOverflow"], | ||
| classes: ["bookTitle"], | ||
| }, | ||
| { | ||
| dataBook: "smallCoverCredits", | ||
| dataDerived: "", | ||
| label: "Cover Credits", | ||
| readOnly: false, | ||
| editableClasses: ["smallCoverCredits"], | ||
| classes: [], | ||
| }, | ||
| { | ||
| dataBook: "", | ||
| dataDerived: "languagesOfBook", | ||
| label: "Languages", | ||
| readOnly: true, | ||
| editableClasses: [], | ||
| classes: ["coverBottomLangName", "Cover-Default-style"], | ||
| }, | ||
| { | ||
| dataBook: "", | ||
| dataDerived: "topic", | ||
| label: "Topic", | ||
| readOnly: true, | ||
| editableClasses: [], | ||
| classes: [ | ||
| "coverBottomBookTopic", | ||
| "bloom-userCannotModifyStyles", | ||
| "bloom-alwaysShowBubble", | ||
| "Cover-Default-style", | ||
| ], | ||
| hint: "Click to choose topic", | ||
| functionOnHintClick: "showTopicChooser", | ||
| }, | ||
| ]; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Also 'become background", Field, and Language menu options for various kinds of canvas elements
JohnThomson
force-pushed
the
customCover3
branch
from
c3f8b03 to
850e89e
Compare
1 participant
This change is