Skip to content

BL-15958 Edge to Edge#7713

Open
hatton wants to merge 8 commits intomasterfrom
BL-15958-edge-to-edge-theme
Open

BL-15958 Edge to Edge#7713
hatton wants to merge 8 commits intomasterfrom
BL-15958-edge-to-edge-theme

Conversation

@hatton
Copy link
Member

@hatton hatton commented Feb 27, 2026

edge-to-edge theme work


Open with Devin

This change is Reviewable

Copilot AI review requested due to automatic review settings February 27, 2026 15:29
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 3 additional findings in Devin Review.

Open in Devin Review

Comment on lines +4 to 14
.bloom-frontMatter,
.bloom-backMatter {
--page-margin: 3mm;
}

/* This statement sets the margins on the numbered content pages */

:not(.bloom-interactive-page).numberedPage.Device16x9Landscape,
:not(.bloom-interactive-page).numberedPage.Device16x9Portrait {
.numberedPage:not(.bloom-interactive-page) {
--page-margin: 0mm;

/* instead of a gap, we are using padding because the text should look centered between the image and edge of the screen */

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 CSS selector broadening from Device-specific to all pages may affect print layouts

The old appearance-theme-zero-margin-ebook.css scoped zero-margin rules to device-specific classes like [class*="Device"] and explicitly listed Device16x9Landscape/Device16x9Portrait. The new appearance-theme-edge-to-edge.css:11 broadens the selector from :not(.bloom-interactive-page).numberedPage.Device16x9Landscape, :not(.bloom-interactive-page).numberedPage.Device16x9Portrait to just .numberedPage:not(.bloom-interactive-page). Similarly, lines 4-5 change from [class*="Device"].bloom-frontMatter to just .bloom-frontMatter. This means the edge-to-edge theme now also applies zero margins and 3mm front/back-matter margins to print page layouts (A5, Letter, etc.), not just device/ebook layouts. This appears intentional based on the PR title "Edge to Edge" (expanding beyond ebook-only), but it's a significant behavioral change worth confirming with the reviewer.

(Refers to lines 4-17)

Open in Devin Review

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

This comment was marked as resolved.

Rename the old zero-margin ebook theme to the new edge-to-edge theme and wire it through templates, migrations, and UI settings.

- Add canonical theme naming in AppearanceSettings with legacy alias normalization so existing books using zero-margin-ebook continue to load as edge-to-edge.
- Rename the theme CSS file to appearance-theme-edge-to-edge.css and refine edge-to-edge/full-bleed layout variables, including page-number placement and trim-offset behavior.
- Update template and migration appearance.json files to use cssThemeName=edge-to-edge.
- Update book settings help text to make full-bleed guidance explicit for Edge to Edge usage.
- Refresh localization entries for the theme label and related strings.
- Update tests to validate alias normalization and edge-to-edge behavior expectations.
@hatton hatton force-pushed the BL-15958-edge-to-edge-theme branch from 40ae5d4 to 5b67c89 Compare February 28, 2026 16:35
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 3 additional findings in Devin Review.

Open in Devin Review

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Missing CSS theme file for 'narrow-margin-ebook' (pre-existing)

The localization files reference AppearanceTheme.narrow-margin-ebook as a theme option, but the src/content/appearanceThemes/ directory only contains: appearance-theme-default.css, appearance-theme-edge-to-edge.css, appearance-theme-legacy-5-6.css, appearance-theme-rounded-border-ebook.css, and a future/ subdirectory. There is no appearance-theme-narrow-margin-ebook.css. This appears to be a pre-existing issue (possibly the theme lives in the future/ directory or is conditionally included), but it's worth confirming that selecting "Narrow Margin Ebook" in the UI doesn't result in a silent no-op for the theme CSS.

Open in Devin Review

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

hatton added 4 commits March 1, 2026 16:59
Align left/right page-number positioning with text edge insets for the edge-to-edge theme, including full-bleed adjustments. Also switch full-bleed bottom handling to an additive variable to avoid recursive custom-property behavior that could hide page numbers.
Introduce a generated page-size lookup (in mm) and centralize paper layout resolution in SizeAndOrientation. Update PDF and EPUB sizing paths to use the shared lookup, broaden supported paper sizes for full-bleed calculations, and add focused tests for GetFullBleedPageSize.
Copy link

@devin-ai-integration devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 4 new potential issues.

View 6 additional findings in Devin Review.

Open in Devin Review

<PackageReference Include="SIL.Windows.Forms.WritingSystems" Version="18.0.0-beta0001" />
<PackageReference Include="SIL.WritingSystems" Version="18.0.0-beta0001" />
<PackageReference Include="sillsdev.dotImpose" Version="2.6.2" />
<PackageReference Include="sillsdev.dotImpose" Version="2.6.2-local.202603011446" />

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 sillsdev.dotImpose version is a local pre-release build

The package reference changed to 2.6.2-local.202603011446 with extra indentation. The -local suffix and timestamp (March 1, 2026) strongly suggest this is a locally-built package not published to nuget.org. The newly added NuGet.Config only lists nuget.org as a source. If this package isn't available from that feed, other developers and CI would fail to restore packages. This may be intentional for development/testing but should not ship in the final PR.

Open in Devin Review

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

devin-ai-integration[bot]

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants