add fit-content support for code block width to theme#6
add fit-content support for code block width to theme#6seven1240 wants to merge 1 commit intoliyown:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds configurable code block widths to the marknative theme so code blocks can render narrower than the full content width (including a fit-content option), with supporting parsing logic and tests.
Changes:
- Extend
Theme.blocks.codeto include awidthoption (defaulting to'100%'). - Add width parsing (
number | % | 'fit-content') and apply it to code block layout sizing. - Add unit tests for
parseWidth.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/marknative/tests/layout/blocks/parseWidth.test.ts | Adds tests for the new width parsing behavior (currently via a copied helper). |
| packages/marknative/src/theme/default-theme.ts | Adds blocks.code.width to the theme type and default theme values. |
| packages/marknative/src/layout/block/layout-document.ts | Introduces parseWidth and applies it to code block fragment sizing (including fit-content). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const width = context.theme.blocks.code.width | ||
| const lineHeight = context.theme.typography.code.lineHeight | ||
| const lineWidth = Math.max(1, context.width - padding * 2) | ||
| const codeTheme = withBodyTypography(context.theme, context.theme.typography.code) |
There was a problem hiding this comment.
layoutCode calculates lineWidth from context.width before applying the new theme.blocks.code.width. If a theme sets code width smaller than the container (e.g. 50% or a number), the code lines will be laid out with a wider max width than the final box, so lines can extend outside box.width (background/outline) and violate the existing invariant that line.width <= box.width - padding*2. Consider computing the desired box width first (for non-fit-content values) and deriving lineWidth from that width (fall back to container width only for fit-content).
| function parseWidth(width: number | string, containerWidth: number): number { | ||
| if (typeof width === 'number') { | ||
| return width > 0 ? width : containerWidth | ||
| } | ||
| if (width === 'fit-content') { | ||
| return -1 |
There was a problem hiding this comment.
parseWidth uses -1 as a sentinel for 'fit-content' but the function is typed to return number, making the contract implicit and easy to misuse later. Consider returning null/undefined (and updating the return type) or a tagged union so callers are forced to handle the 'fit-content' case explicitly.
| function parseWidth(width: number | string, containerWidth: number): number { | |
| if (typeof width === 'number') { | |
| return width > 0 ? width : containerWidth | |
| } | |
| if (width === 'fit-content') { | |
| return -1 | |
| function parseWidth(width: number | string, containerWidth: number): number | undefined { | |
| if (typeof width === 'number') { | |
| return width > 0 ? width : containerWidth | |
| } | |
| if (width === 'fit-content') { | |
| return undefined |
| if (typeof width === 'number') { | ||
| return width > 0 ? width : containerWidth | ||
| } |
There was a problem hiding this comment.
parseWidth currently allows numeric widths greater than containerWidth (and percentages > 100%) to pass through unchanged, which can cause code blocks to render outside the page content area. If overflow isn’t intended, consider clamping parsed widths to [1, containerWidth] (and similarly clamping computed percentages).
| heading: { marginTop: number; marginBottom: number } | ||
| list: { marginBottom: number; itemGap: number; indent: number } | ||
| code: { marginBottom: number; padding: number } | ||
| code: { marginBottom: number; padding: number, width: number | string } |
There was a problem hiding this comment.
Adding blocks.code.width as a required property in the exported Theme type is a breaking change for downstream consumers that construct Theme objects directly. Consider making it optional (width?: …) with a default fallback to full container width in layout, and keep the type literal formatting consistent (use ; between properties rather than ,).
| code: { marginBottom: number; padding: number, width: number | string } | |
| code: { marginBottom: number; padding: number; width?: number | string } |
| import { test, expect } from "bun:test"; | ||
|
|
||
| // Copy parseWidth function for testing | ||
| function parseWidth(width: number | string, containerWidth: number): number { | ||
| if (typeof width === 'number') { | ||
| return width > 0 ? width : containerWidth | ||
| } | ||
| if (width === 'fit-content') { | ||
| return -1 // Special marker for caller to handle | ||
| } | ||
| // Support percentage format like "50%", "80%" | ||
| const match = width.match(/^(\d+(?:\.\d+)?)%$/) | ||
| if (match && match[1]) { | ||
| const percentage = parseFloat(match[1]) | ||
| if (!isNaN(percentage) && percentage > 0) { | ||
| return (containerWidth * percentage) / 100 | ||
| } | ||
| } | ||
| // Default to container width | ||
| return containerWidth | ||
| } | ||
|
|
||
| test("parseWidth: number > 0 returns the number itself", () => { | ||
| expect(parseWidth(100, 500)).toBe(100) | ||
| expect(parseWidth(300, 500)).toBe(300) | ||
| }) | ||
|
|
||
| test("parseWidth: number <= 0 returns container width", () => { | ||
| expect(parseWidth(0, 500)).toBe(500) | ||
| expect(parseWidth(-10, 500)).toBe(500) | ||
| }) | ||
|
|
||
| test("parseWidth: fit-content returns -1", () => { | ||
| expect(parseWidth('fit-content', 500)).toBe(-1) | ||
| }) | ||
|
|
||
| test("parseWidth: percentage strings", () => { | ||
| expect(parseWidth('50%', 500)).toBe(250) | ||
| expect(parseWidth('100%', 500)).toBe(500) | ||
| expect(parseWidth('25%', 400)).toBe(100) | ||
| }) | ||
|
|
||
| test("parseWidth: decimal percentages", () => { | ||
| expect(parseWidth('33.33%', 300)).toBeCloseTo(99.99) | ||
| expect(parseWidth('66.66%', 300)).toBeCloseTo(199.98) | ||
| }) | ||
|
|
||
| test("parseWidth: invalid percentages return container width", () => { | ||
| expect(parseWidth('abc', 500)).toBe(500) | ||
| expect(parseWidth('50', 500)).toBe(500) | ||
| expect(parseWidth('50 px', 500)).toBe(500) | ||
| expect(parseWidth('', 500)).toBe(500) | ||
| }) |
There was a problem hiding this comment.
This test file copies parseWidth instead of testing the implementation used by layoutDocument/layoutCode, so it can pass even if the production logic regresses or diverges. Consider moving parseWidth into a shared exported utility (or testing via layoutDocument with a theme override like blocks.code.width: 'fit-content' | '50%') so the tests validate the actual behavior change end-to-end.
| import { test, expect } from "bun:test"; | |
| // Copy parseWidth function for testing | |
| function parseWidth(width: number | string, containerWidth: number): number { | |
| if (typeof width === 'number') { | |
| return width > 0 ? width : containerWidth | |
| } | |
| if (width === 'fit-content') { | |
| return -1 // Special marker for caller to handle | |
| } | |
| // Support percentage format like "50%", "80%" | |
| const match = width.match(/^(\d+(?:\.\d+)?)%$/) | |
| if (match && match[1]) { | |
| const percentage = parseFloat(match[1]) | |
| if (!isNaN(percentage) && percentage > 0) { | |
| return (containerWidth * percentage) / 100 | |
| } | |
| } | |
| // Default to container width | |
| return containerWidth | |
| } | |
| test("parseWidth: number > 0 returns the number itself", () => { | |
| expect(parseWidth(100, 500)).toBe(100) | |
| expect(parseWidth(300, 500)).toBe(300) | |
| }) | |
| test("parseWidth: number <= 0 returns container width", () => { | |
| expect(parseWidth(0, 500)).toBe(500) | |
| expect(parseWidth(-10, 500)).toBe(500) | |
| }) | |
| test("parseWidth: fit-content returns -1", () => { | |
| expect(parseWidth('fit-content', 500)).toBe(-1) | |
| }) | |
| test("parseWidth: percentage strings", () => { | |
| expect(parseWidth('50%', 500)).toBe(250) | |
| expect(parseWidth('100%', 500)).toBe(500) | |
| expect(parseWidth('25%', 400)).toBe(100) | |
| }) | |
| test("parseWidth: decimal percentages", () => { | |
| expect(parseWidth('33.33%', 300)).toBeCloseTo(99.99) | |
| expect(parseWidth('66.66%', 300)).toBeCloseTo(199.98) | |
| }) | |
| test("parseWidth: invalid percentages return container width", () => { | |
| expect(parseWidth('abc', 500)).toBe(500) | |
| expect(parseWidth('50', 500)).toBe(500) | |
| expect(parseWidth('50 px', 500)).toBe(500) | |
| expect(parseWidth('', 500)).toBe(500) | |
| }) | |
| import { test } from "bun:test"; | |
| test.todo( | |
| "layoutDocument/layoutCode applies a positive numeric blocks.code.width as an explicit width", | |
| ); | |
| test.todo( | |
| "layoutDocument/layoutCode falls back to the container width when blocks.code.width is 0 or negative", | |
| ); | |
| test.todo( | |
| "layoutDocument/layoutCode preserves fit-content behavior when blocks.code.width is 'fit-content'", | |
| ); | |
| test.todo( | |
| "layoutDocument/layoutCode supports percentage widths such as '50%' and '100%' via the production parser", | |
| ); | |
| test.todo( | |
| "layoutDocument/layoutCode supports decimal percentage widths such as '33.33%' via the production parser", | |
| ); | |
| test.todo( | |
| "layoutDocument/layoutCode falls back to the container width for invalid width strings", | |
| ); |
| @@ -0,0 +1,53 @@ | |||
| import { test, expect } from "bun:test"; | |||
There was a problem hiding this comment.
This new test doesn’t follow the established test style in this package (single quotes and no trailing semicolons, e.g. tests/layout/blocks/layout-blocks.test.ts:1). To keep formatting consistent (and avoid formatter churn), update imports and test names to use single quotes and drop semicolons.
|
could you give me some example render image for different width |
the project is great and sometimes I want the width of the code block smaller than 100%. This patch works for me. Hope it helpful.