Skip to content

feat: Add percentage width support for two-column layouts#20

Merged
orangewise merged 6 commits intomainfrom
claude/add-column-width-percentage-d1ebw
Jan 13, 2026
Merged

feat: Add percentage width support for two-column layouts#20
orangewise merged 6 commits intomainfrom
claude/add-column-width-percentage-d1ebw

Conversation

@orangewise
Copy link
Copy Markdown
Owner

This commit adds the ability to specify custom width percentages for
two-column layouts, allowing more flexible column sizing beyond the
default 50/50 split.

Changes:

  • Parser: Support :::columns[60] syntax to specify left column width

    • The right column automatically gets the remaining width
    • Backward compatible: :::columns still works for 50/50 split
    • Width is included in marker format: <!-- COLUMN:LEFT:START:60 -->
  • Frontend: Extract and apply width styles from markers

    • Parse width percentage from column markers
    • Apply inline flex styles for custom widths
    • Validate width is between 1-99%
  • Examples: Add four new example slides demonstrating the feature

    • Explanation slide for the syntax
    • 70/30 split example (wide main content + sidebar)
    • 60/40 split example (code review layout)
    • 30/70 split example (narrow left + wide right)
  • Tests: Add comprehensive unit tests

    • Test various percentage values (30%, 50%, 60%, 70%)
    • Test backward compatibility (columns without width)
    • Test integration with other features (notes, width modes)
    • Test multiple column blocks with different widths
  • Screenshots: Update capture script to include new examples

    • Add screenshots for 70/30 and 30/70 layouts
    • Will be generated automatically by GitHub Actions

All 36 tests pass successfully.

claude and others added 2 commits January 12, 2026 19:55
This commit adds the ability to specify custom width percentages for
two-column layouts, allowing more flexible column sizing beyond the
default 50/50 split.

Changes:
- Parser: Support `:::columns[60]` syntax to specify left column width
  - The right column automatically gets the remaining width
  - Backward compatible: `:::columns` still works for 50/50 split
  - Width is included in marker format: `<!-- COLUMN:LEFT:START:60 -->`

- Frontend: Extract and apply width styles from markers
  - Parse width percentage from column markers
  - Apply inline flex styles for custom widths
  - Validate width is between 1-99%

- Examples: Add four new example slides demonstrating the feature
  - Explanation slide for the syntax
  - 70/30 split example (wide main content + sidebar)
  - 60/40 split example (code review layout)
  - 30/70 split example (narrow left + wide right)

- Tests: Add comprehensive unit tests
  - Test various percentage values (30%, 50%, 60%, 70%)
  - Test backward compatibility (columns without width)
  - Test integration with other features (notes, width modes)
  - Test multiple column blocks with different widths

- Screenshots: Update capture script to include new examples
  - Add screenshots for 70/30 and 30/70 layouts
  - Will be generated automatically by GitHub Actions

All 36 tests pass successfully.
@claude
Copy link
Copy Markdown

claude bot commented Jan 12, 2026

Code Review: Custom Column Width Percentage Feature

Summary

This PR adds custom width percentages for two-column layouts. Overall, this is a well-implemented feature with good test coverage and documentation.

Strengths

  1. Excellent Test Coverage - 8 new comprehensive test cases
  2. Backward Compatibility - Existing presentations won't break
  3. Clear Documentation - 4 example slides and updated docstrings
  4. Consistent Architecture - Follows existing patterns

Issues Found

1. Width Validation Inconsistency (Minor)

Location: markdeck/static/slides.js:306-307

The frontend validates width (1-99%), but the backend parser has NO validation. Users could write columns[150] and it would be accepted.

Recommendation: Add validation in parser.py after line 121

2. Regex Doesn't Validate Width Format (Minor)

Location: markdeck/parser.py:118

Pattern accepts columns[000060] or columns[9999]. Update pattern to limit to 1-99 range.

3. CSS Flex Behavior Could Be Clearer

Location: markdeck/static/slides.js:305-310

Right column relies on implicit flex calculation. Making it explicit would improve maintainability by calculating rightWidth = 100 - validWidth.

Security and Performance

  • No XSS concerns
  • No performance issues

Recommendations

Priority High:

  1. Add backend width validation in parser.py
  2. Update regex pattern to limit width to 1-99 range

Priority Medium:
3. Make right column flex explicit
4. Add test cases for invalid widths

Approval Status

APPROVED with minor suggestions

This is a high-quality PR that solves a real user need with excellent test coverage. The suggested improvements are minor and could be addressed in a follow-up PR. The core functionality is solid and ready to merge.

Great work on this feature!

Review conducted following CLAUDE.md guidelines

@orangewise
Copy link
Copy Markdown
Owner Author

Replace column_width_70_30.png with a screenshot of Custom Width Example: 30/70 Narrow Left

@claude

Plus bump version and update the readme

@claude
Copy link
Copy Markdown

claude bot commented Jan 13, 2026

Claude finished @orangewise's task in 2m 35s —— View job


All requested changes completed! ✅

Task List

  • Understand the screenshot requirements and current state
  • Update capture script to screenshot 30/70 slide for column_width_70_30.png
  • Bump version from 0.5.0 to 0.6.0
  • Update README.md with custom column width documentation
  • Commit and push changes

Changes Made

  1. Screenshot Script (capture_screenshots.py):

    • Updated to capture slide 31 (30/70 Narrow Left) for column_width_70_30.png
    • The filename keeps the 70_30 name but will show the 30/70 slide as requested
    • Note: The actual screenshot file will be regenerated by GitHub Actions workflow
  2. Version Bump:

    • Updated version from 0.5.0 to 0.6.0 in:
      • pyproject.toml
      • markdeck/__init__.py
  3. README Documentation:

    • Added comprehensive "Custom Column Widths" section with:
      • Syntax explanation (:::columns[width])
      • Features and backward compatibility notes
      • Common use cases (70/30, 60/40, 30/70 splits)
      • Live code examples
    • Updated roadmap to mark custom column widths as complete ✓

All changes have been committed and pushed to the PR branch.

--- |

…custom column widths

Changes:
- Update capture_screenshots.py to screenshot 30/70 slide for column_width_70_30.png
- Bump version from 0.5.0 to 0.6.0 in pyproject.toml and __init__.py
- Add comprehensive "Custom Column Widths" section to README
- Update roadmap to mark custom column widths as complete

Co-authored-by: Ronald Luitwieler <orangewise@users.noreply.github.com>
@claude
Copy link
Copy Markdown

claude bot commented Jan 13, 2026

Code Review: Custom Column Width Feature

Overview

This PR adds support for custom percentage-based column widths to the two-column layout feature. The implementation is well-designed, thoroughly tested, and ready to merge with a few minor suggestions below.


✅ Strengths

1. Excellent Backward Compatibility

  • The optional width syntax (:::columns[60]) preserves existing :::columns behavior perfectly
  • Parser gracefully handles both formats using regex optional group: (?:\[(\d+)\])?
  • Tests explicitly verify backward compatibility (test_parser.py:329-342)

2. Comprehensive Test Coverage

  • 8 new test cases covering edge cases and integration scenarios:
    • Various percentages (30%, 50%, 60%, 70%)
    • Backward compatibility
    • Interaction with markdown features
    • Integration with speaker notes and wide mode
    • Multiple column blocks with different widths
  • All tests follow existing patterns and have clear docstrings

3. Clean Implementation

  • Parser (markdeck/parser.py:117-146): Minimal changes, reuses existing column transformation logic
  • Frontend (markdeck/static/slides.js:263-322): Elegant width extraction and validation
  • Both components maintain separation of concerns

4. Good Documentation

  • Updated README with syntax examples, use cases, and feature notes
  • Added 4 example slides demonstrating different width ratios
  • Updated screenshot capture script
  • Clear inline comments explaining the width marker format

5. Proper Validation

  • Frontend validates width between 1-99%: Math.max(1, Math.min(99, leftWidthPercent)) (slides.js:307)
  • Prevents invalid layouts and edge cases

🔍 Areas for Consideration

1. Input Validation Gap ⚠️

Issue: Parser accepts any digit sequence in :::columns[width] without validation.

Current behavior:

column_pattern = r":::columns(?:\[(\d+)\])?\s*\n(.*?)\s*\n:::"  # Line 119
# Accepts: :::columns[0], :::columns[100], :::columns[999]

Potential problems:

  • :::columns[0] → Frontend clamps to 1%, but unusual semantics
  • :::columns[100] → Frontend clamps to 99%, right column gets 1px
  • :::columns[999] → Frontend clamps to 99%, but confusing for users

Recommendation: Add validation in parser to match frontend constraints:

def replace_columns(match):
    width_percent = match.group(1)
    if width_percent:
        width = int(width_percent)
        if not (1 <= width <= 99):
            # Option 1: Clamp silently
            width = max(1, min(99, width))
            width_percent = str(width)
            # Option 2: Warn user (better UX)
            print(f"Warning: Column width {width} clamped to 1-99 range")
    # ... rest of logic

Priority: Medium (nice-to-have for better UX and error messages)

2. Edge Case: Multiple Columns on Same Slide

Observation: Test covers multiple column blocks (test_parser.py:408-431), which works correctly.

Question: Should documentation clarify expected behavior? Currently:

  • Each block can have different widths ✓
  • Blocks are independent ✓

Recommendation: Add note to README about multiple column blocks (optional, already tested).

3. Screenshot Naming Inconsistency (Minor)

File: capture_screenshots.py:167

# Comment says 30/70 but filename is 70_30
# Note: Using 70_30 filename but showing 30/70 slide per user request
await page.screenshot(path="screenshots/column_width_70_30.png")

Impact: Low (filenames don't affect functionality)

Suggestion: Either:

  1. Use consistent naming: column_width_30_70.png for 30% left / 70% right
  2. Update comment to clarify naming convention

4. Version Bump

Observation: Version bumped from 0.5.0 → 0.6.0 (minor version)

Correct choice per semantic versioning:

  • Adds new feature (column width percentage)
  • Maintains backward compatibility
  • No breaking changes

🛡️ Security & Performance

Security

No vulnerabilities detected:

  • No user input reaches eval/exec
  • Regex patterns are safe (no ReDoS risk)
  • Frontend validation prevents injection via inline styles
  • Width is parsed as integer and clamped before use

Performance

Minimal impact:

  • Regex change adds optional group (negligible cost)
  • Frontend adds one regex match per slide (cached result)
  • No additional DOM operations or reflows

📋 Code Style & Conventions

Follows CLAUDE.md guidelines:

  • Python: snake_case, clear variable names, proper docstrings
  • JavaScript: camelCase, regex patterns, clear comments
  • Line length within 100 chars
  • Consistent with existing codebase patterns

Git workflow:

  • Branch naming: claude/add-column-width-percentage-d1ebw
  • Clear commit message with detailed body ✓
  • All tests passing ✓

🎯 Recommendations Summary

Priority Issue Action Effort
Medium Parser validation Add 1-99% range check with warning 5-10 min
Low Screenshot naming Clarify naming convention in comment 2 min
Optional Documentation Note about multiple column blocks 5 min

✨ Final Verdict

Status: ✅ Approved - Ready to Merge

This is a high-quality implementation that:

  • Solves a real use case (asymmetric layouts)
  • Maintains backward compatibility
  • Has excellent test coverage
  • Follows project conventions
  • Includes documentation and examples

The suggestions above are optional refinements - the PR is production-ready as-is.

Recommendation: Merge now, optionally address parser validation in a follow-up PR.


🎉 Great Work!

Special kudos for:

  • Thorough testing (8 new test cases!)
  • Backward compatibility testing
  • Real-world examples (code review, sidebar, stats)
  • Clean, minimal code changes

Generated with Claude Code Review

@orangewise
Copy link
Copy Markdown
Owner Author

Thanks, i think you missed this one "Replace column_width_70_30.png with a screenshot of Custom Width Example: 30/70 Narrow Left"

@claude

@claude
Copy link
Copy Markdown

claude bot commented Jan 13, 2026

Claude finished @orangewise's task in 1m 8s —— View job


Screenshot Capture Fixed ✅

Task List

  • Investigate current screenshot capture configuration
  • Find correct slide number for "30/70 Narrow Left" example
  • Update capture script with correct slide number
  • Commit and push fix

Changes Made

Fixed the slide navigation in capture_screenshots.py:

  • Changed from navigating to slide 31 → slide 32
  • This correctly captures the "Custom Width Example: 30/70 Narrow Left" slide
  • The screenshot will be saved as column_width_70_30.png (keeping the filename as requested)

The GitHub Actions workflow will automatically regenerate the screenshot when it runs next.

--- | Branch | Latest commit

Navigate to slide 32 (not 31) to capture the 'Custom Width Example: 30/70 Narrow Left' slide for column_width_70_30.png.

Co-authored-by: Ronald Luitwieler <orangewise@users.noreply.github.com>
@claude
Copy link
Copy Markdown

claude bot commented Jan 13, 2026

Code Review: Add percentage width support for two-column layouts

Summary

This is a well-implemented feature that adds custom width percentages to two-column layouts. The implementation is clean, backward compatible, and includes comprehensive test coverage.

✅ Strengths

1. Clean Architecture

  • Parser and frontend responsibilities are well-separated
  • Backward compatibility preserved (:::columns still works)
  • 150 lines of tests covering various scenarios

2. Excellent Test Coverage

Tests cover: percentage values (30%, 50%, 60%, 70%), backward compatibility, integration with notes/width modes, multiple column blocks, and edge cases.

3. Good Documentation

README updated with clear examples and use cases. Code comments explain transformation logic.

🔍 Issues & Concerns

1. Missing Input Validation in Parser (Medium Priority)

Location: markdeck/parser.py:119-136

The parser accepts any digit sequence without validation. Invalid values like :::columns[0], :::columns[100], or :::columns[999] pass through to frontend.

Recommendation: Add validation after line 122 to check width is 1-99, fallback to default if invalid.

2. Silent Value Clamping (Low Priority)

Location: markdeck/static/slides.js:307

Frontend silently clamps invalid values without warning. Consider adding console.warn() for values outside 1-99 range.

3. Screenshot Naming Confusion (Low Priority)

Location: capture_screenshots.py:167

Comment says "Using 70_30 filename but showing 30/70 slide". Both column_width_30_70.png and column_width_70_30.png appear identical (same binary). Verify correct slides are captured.

🎯 Code Quality

Parser (Python):

  • ✅ Clean regex with non-capturing group
  • ✅ Maintains code block protection
  • ⚠️ Missing input validation

Frontend (JavaScript):

  • ✅ Proper parseInt usage with radix
  • ✅ Good error handling for malformed markers
  • ⚠️ Silent clamping could hide user errors

Tests:

  • ✅ Comprehensive coverage
  • ✅ Clear naming conventions
  • ✅ Integration tests included

🔒 Security

✅ No security concerns - values validated/clamped, no XSS risks, no injection risks

⚡ Performance

✅ Minimal impact - efficient regex, O(1) validation, no extra DOM manipulation

📋 Recommendations

  1. Add parser-side validation for width percentages (1-99 range)
  2. Consider console warnings for clamped values
  3. Verify screenshot files match intended slides
  4. Optional: Add markdeck validate check for invalid widths

🎉 Conclusion

High-quality PR with excellent test coverage and documentation. Identified issues are minor and don't block merging. Feature works correctly even with current validation.

Recommendation: ✅ Approve with minor suggestions

Great work on this feature! The 70/30 and 60/40 split examples are particularly useful for common presentation layouts.

@orangewise orangewise merged commit e2f4bed into main Jan 13, 2026
@orangewise orangewise deleted the claude/add-column-width-percentage-d1ebw branch January 13, 2026 15:53
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