AutoSize blocks on custom page when field or lang changes (BL-15996)#7748
AutoSize blocks on custom page when field or lang changes (BL-15996)#7748JohnThomson wants to merge 2 commits 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.
| const removeConflictingStyleClasses = ( | ||
| fieldType: { | ||
| editableClasses: string[]; | ||
| classes: string[]; | ||
| }, | ||
| editables: HTMLElement[], | ||
| ) => { | ||
| const newStyleClasses = new Set( | ||
| [...fieldType.classes, ...fieldType.editableClasses].filter((c) => | ||
| c.endsWith("-style"), | ||
| ), | ||
| ); | ||
| if (newStyleClasses.size === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const stripStyleClasses = (element: HTMLElement) => { | ||
| Array.from(element.classList).forEach((className) => { | ||
| if ( | ||
| className.endsWith("-style") && | ||
| !newStyleClasses.has(className) | ||
| ) { | ||
| element.classList.remove(className); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| stripStyleClasses(tg); | ||
| editables.forEach((editable) => stripStyleClasses(editable)); | ||
| }; |
There was a problem hiding this comment.
🚩 removeConflictingStyleClasses strips all non-matching -style classes, including non-field-type ones
The new removeConflictingStyleClasses function at line 1255-1284 removes ALL -style classes from both tg and editables that aren't in the new field type's set. This goes beyond what clearFieldTypeClasses does (which only removes known field-type classes). If an editable had a pre-existing style class like Normal-style from before any field type was set, switching to e.g. bookTitle would permanently remove it. Switching back to "None" field type would not restore it, since the "None" handler only calls clearFieldTypeClasses() without adding any style class back. This is likely the intended behavior (ensuring the field type's style takes full precedence), but it means the style class removal is one-way and irreversible through the UI.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
There's no way to get back to the class it might have had before the Field was first set, unless we save it in some data- attribute. Having more than one -style class on an element is asking for trouble (e.g., which one would style editor let you edit?), so I think removing all the others is necessary. I did add restoring the usual default style for comic bubbles, since I'm not sure what might fail if there is not at least one -style class. We might still want some improvement here, but I think we need users to tell us what.
JohnThomson
force-pushed
the
autosizeOnFieldLang
branch
from
b927817 to
ac0e951
Compare
| )) { | ||
| editable.removeAttribute("data-book"); | ||
| } | ||
| adjustAutoSizeForVisibleEditableInTranslationGroup(tg); | ||
| setMenuOpen(false); |
There was a problem hiding this comment.
🚩 Appearance classes not cleaned up when switching field type to 'None'
The new applyAppearanceClassForEditable function (line 998-1011) adds bloom-contentFirst/bloom-contentSecond/bloom-contentThird classes when switching TO a field type controlled by the appearance system (e.g., bookTitle). However, when switching FROM such a field type to 'None' (lines 1303-1311), clearFieldTypeClasses() does not remove these appearance classes — it only removes the classes explicitly listed in fieldTypeData. This means bloom-contentFirst etc. could persist as stale classes. The practical impact is likely minimal since these classes control language-role-based styling that may still be appropriate regardless of field type, but it's an asymmetry in the new code: appearance classes are added by applyAppearanceClassForEditable but never explicitly removed when no longer relevant.
(Refers to lines 1303-1311)
Was this helpful? React with 👍 or 👎 to provide feedback.
Cleaning up unwanted -style classes, fetching content when setting language of an element that already has a field type, using wrapper for async DOM changes
JohnThomson
force-pushed
the
autosizeOnFieldLang
branch
from
ac0e951 to
4f6f250
Compare
StephenMcConnel
left a comment
StephenMcConnel
left a comment
There was a problem hiding this comment.
@StephenMcConnel reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on JohnThomson).
JohnThomson
left a comment
JohnThomson
left a comment
There was a problem hiding this comment.
@JohnThomson made 2 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on JohnThomson).
| const removeConflictingStyleClasses = ( | ||
| fieldType: { | ||
| editableClasses: string[]; | ||
| classes: string[]; | ||
| }, | ||
| editables: HTMLElement[], | ||
| ) => { | ||
| const newStyleClasses = new Set( | ||
| [...fieldType.classes, ...fieldType.editableClasses].filter((c) => | ||
| c.endsWith("-style"), | ||
| ), | ||
| ); | ||
| if (newStyleClasses.size === 0) { | ||
| return; | ||
| } | ||
|
|
||
| const stripStyleClasses = (element: HTMLElement) => { | ||
| Array.from(element.classList).forEach((className) => { | ||
| if ( | ||
| className.endsWith("-style") && | ||
| !newStyleClasses.has(className) | ||
| ) { | ||
| element.classList.remove(className); | ||
| } | ||
| }); | ||
| }; | ||
|
|
||
| stripStyleClasses(tg); | ||
| editables.forEach((editable) => stripStyleClasses(editable)); | ||
| }; |
There was a problem hiding this comment.
There's no way to get back to the class it might have had before the Field was first set, unless we save it in some data- attribute. Having more than one -style class on an element is asking for trouble (e.g., which one would style editor let you edit?), so I think removing all the others is necessary. I did add restoring the usual default style for comic bubbles, since I'm not sure what might fail if there is not at least one -style class. We might still want some improvement here, but I think we need users to tell us what.
| )) { | ||
| editable.removeAttribute("data-book"); | ||
| } | ||
| adjustAutoSizeForVisibleEditableInTranslationGroup(tg); | ||
| setMenuOpen(false); |
StephenMcConnel
left a comment
StephenMcConnel
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on JohnThomson).
2 participants
This change is