chore: remove jenkins CODE_VERSION param#31
Merged
TomWoodward merged 1 commit intomasterfrom Dec 3, 2018
Merged
Conversation
TomWoodward
approved these changes
Dec 3, 2018
OpenStaxClaude
added a commit
that referenced
this pull request
Mar 27, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
7 tasks
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 27, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 27, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 30, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Update index.css Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 31, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Update index.css Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 31, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Update index.css Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 31, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Update index.css Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 1, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Update index.css Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
OpenStaxClaude
added a commit
that referenced
this pull request
Apr 6, 2026
The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 7, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 8, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Debugging back in Add debugging to show all link hrefs before clicking Add debugging to investigate URL and selector formation Clean up debugging Add non-null assertions for TypeScript strict null checks in browser test TypeScript was complaining that document and window might be undefined within the page.evaluate() callback. However, within a browser context (which is where page.evaluate() runs), these globals are guaranteed to exist. Added non-null assertions (!) to satisfy TypeScript's strict null checking: - document.querySelector - document.querySelectorAll - document.documentElement - window.pageYOffset - window.getComputedStyle - window.matchMedia Addresses Review #41 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Debugging back in Add debugging to show all link hrefs before clicking Add debugging to investigate URL and selector formation Clean up debugging Add non-null assertions for TypeScript strict null checks in browser test TypeScript was complaining that document and window might be undefined within the page.evaluate() callback. However, within a browser context (which is where page.evaluate() runs), these globals are guaranteed to exist. Added non-null assertions (!) to satisfy TypeScript's strict null checking: - document.querySelector - document.querySelectorAll - document.documentElement - window.pageYOffset - window.getComputedStyle - window.matchMedia Addresses Review #41 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
OpenStaxClaude
added a commit
that referenced
this pull request
Apr 9, 2026
1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Update index.css Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 9, 2026
Use dynamic scroll position calculations in browser tests Replace hardcoded EXPECTED_SCROLL_TOPS values with dynamic position calculations. The test now: 1. Calculates expected scroll positions based on actual element positions 2. Accounts for sticky header (topbar) height dynamically 3. Eliminates brittleness from hardcoded pixel values that break when CSS changes This approach is more robust and adapts automatically to layout changes from the CSS Modules migration, addressing Reviews #23 and #24. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Allow for undefined window, document Fix scroll position calculation to account for book banner The dynamic scroll position calculation was only accounting for the topbar height, but not the book banner that's also sticky above it. The topbar is positioned at top: [bookBannerHeight]rem, so the total sticky offset is banner height + topbar height. This fix calculates the book banner height from the topbar's computed 'top' value and adds it to the topbar height for accurate scroll position validation. Fixes the ~90-100px discrepancy in Content.browserspec.tsx scroll tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Fix scroll position calculation to use scroll-padding CSS property The browser test was failing with a 30px discrepancy because the expected scroll position calculation was manually computing the sticky header offset instead of using the actual scroll-padding CSS property that the browser uses for anchor navigation. Changed getExpectedScrollPosition() to read the computed scroll-padding-top value from the body element, which already accounts for the complete sticky header stack (book banner + topbar). This matches exactly how the browser positions elements when navigating to hash anchors. Fixes Review #28 - scroll position should now be within 10px threshold 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix TypeScript error: use getPropertyValue for scroll-padding-top The scrollPaddingTop property doesn't exist on CSSStyleDeclaration type. Use getPropertyValue() method to access the CSS property value instead. Addresses Review #30 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to manual sticky offset calculation for browser test The scroll-padding approach introduced in commit 9cc0492 made the test worse (90px and 66px discrepancies vs the previous 30px discrepancy). Reverting to the more accurate manual calculation from commit 9324f0c which explicitly calculates: - Topbar height from getBoundingClientRect() - Book banner height from topbar's computed top value - Total sticky offset = banner height + topbar height This approach directly measures the actual rendered heights rather than relying on CSS scroll-padding which may not account for all layout nuances. Addresses Review #31 feedback. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Simplify page.evaluate() callback to fix execution issue Removed Window type casting and unnecessary complexity that was preventing the page.evaluate() callback from executing properly. The function now: - Uses window and document globals directly (no casting) - Removes type annotations from the callback parameters - Explicitly awaits and logs the result - Adds debug logging inside page.evaluate() to verify execution This should fix the issue where expectedScrollTop was always returning 0 because the callback wasn't being executed. Addresses Review #40 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix page.evaluate serialization in browser test The page.evaluate function was not executing properly because: 1. Arguments were passed as an object requiring destructuring, which doesn't serialize well in Puppeteer 2. Optional chaining (document?., window?.) was unnecessary in browser context Changed to: - Pass arguments as separate parameters directly to page.evaluate - Remove optional chaining operators (document, window are guaranteed to exist) - Add explicit type annotations to function signature This should allow the evaluate callback to execute properly and return expected scroll positions. Addresses Review #37 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Update Content.browserspec.tsx Update utils.ts Update Content.browserspec.tsx Update Content.browserspec.tsx Update Content.browserspec.tsx Fix window reference in browser test (Review #38) The getExpectedScrollPosition function was trying to access window and document in the Node.js test context before calling page.evaluate(). These global objects only exist inside the browser context (within page.evaluate callback). Changes: - Removed window/document access outside page.evaluate - Changed all 'w' references to 'window' inside the evaluate callback - The function now only accesses browser globals within the browser context This fixes the ReferenceError: window is not defined 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add debugging to investigate null element in scroll position test Per Review #36, element is always null in page.evaluate. Added logging to: - Show the selector being passed - List available element IDs in the document when element is null - Help identify if the target elements exist or if selector format is wrong This will help diagnose why querySelector returns null for the target elements. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update Content.browserspec.tsx Add feedback Add delay after clicking links to allow ScrollOffset adjustment Wait 500ms after clicking anchor links to ensure the ScrollOffset component's manual scroll adjustment (via scrollBy) has completed before checking the final scroll position. The ScrollOffset component applies an additional scroll adjustment after the browser's native hash navigation, which may take some time to complete. This delay should help the test wait for the final scroll position to stabilize. Addresses Review #34 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix browser test scroll position calculation for link clicks The ScrollOffset component applies a double-offset for clicked links as a workaround for browsers that don't support scroll-padding properly: 1. Browser scrolls to elementTop - scrollPadding (native behavior) 2. ScrollOffset.clickHandler applies scrollBy(0, -scrollPadding) (manual adjustment) This results in final position: elementTop - 2*scrollPadding Updated getExpectedScrollPosition() to account for this double-offset when isClickNavigation=true. Initial hash navigation (line 93) uses single offset, while link clicks (line 111) use double offset. Addresses Review #33 browser test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix scroll position calculation in browser test Use the actual computed scroll-padding-top CSS property instead of manually calculating sticky header offsets. The ScrollOffset component sets CSS variables --scroll-offset-desktop and --scroll-offset-mobile which are applied via the scroll-padding CSS property on the body element. This is the exact value the browser uses for hash navigation. The manual calculation was missing some offset nuances, resulting in test failures with 90px (desktop) and 66px (mobile) discrepancies. By reading the computed scroll-padding-top value, we now match exactly what the browser does. Addresses review comment #32 🤖 Generated with [Claude Code](https://claude.com/claude-code) Debugging back in Add debugging to show all link hrefs before clicking Add debugging to investigate URL and selector formation Clean up debugging Add non-null assertions for TypeScript strict null checks in browser test TypeScript was complaining that document and window might be undefined within the page.evaluate() callback. However, within a browser context (which is where page.evaluate() runs), these globals are guaranteed to exist. Added non-null assertions (!) to satisfy TypeScript's strict null checking: - document.querySelector - document.querySelectorAll - document.documentElement - window.pageYOffset - window.getComputedStyle - window.matchMedia Addresses Review #41 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
RoyEJohnson
pushed a commit
that referenced
this pull request
Apr 14, 2026
* Phase 2.2: Migrate Modal components to plain CSS Migrates the Modal component system from styled-components to plain CSS modules, following the established patterns from Phase 1.1-1.5. Changes: - Created Modal.css with CSS classes for all modal components - Created Modal.tsx with plain React component wrappers - Created styles.legacy.tsx for backward compatibility - Updated index.tsx to use new CSS-based components - Updated styles.tsx to redirect to legacy exports Components migrated: - ModalWrapper - Full-screen fixed overlay container - CardWrapper - Z-index container - Card - Main modal card with shadow - Header - Header section - Heading - Title with h4 typography - BodyHeading - Secondary heading with h3 typography - Body - Content area - Mask - Semi-transparent backdrop - Footer - Footer section - CloseModalIcon - Close button with icon Maintains backward compatibility for existing consumers: - ErrorModal (imports Body, BodyHeading, Footer, modalPadding) - ConfirmationModal (imports and extends Footer) - Footer ContactDialog (extends Modal) All styled-component dependencies inlined or converted to CSS variables. Related: CORE-1696 * Address Copilot review comments 1. Fixed duplicate padding in .modal-body-heading CSS - Removed the overridden padding declaration (1.5rem 0 1rem 0) - Kept the actual applied padding (1.5rem 0) - Eliminates confusion about which padding value is used 2. Explained why styled() wrappers must be kept - Added comprehensive comment explaining that styled() wrappers are required - Consumers like ConfirmationModal extend components using styled(Footer) - Direct re-exports would break this functionality - Trailing whitespace is already handled by normalizeClassName() helper These changes address the last two unresolved Copilot review comments while maintaining full backward compatibility with existing code. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address Copilot review comments for Modal migration This commit addresses all three issues raised by Copilot's automated review: 1. Add link styling to modal card (.modal-card a) - Mirrors bodyCopyRegularStyle link behavior with underline and hover states - Uses link color constants from Typography (linkColor, linkHover) - Binds link colors as CSS variables in Card component 2. Fix CloseModalIcon size for pixel-perfect parity - Changed from 2.5rem to 2rem (height and width) - Matches the final size from legacy styled-components implementation 3. Ensure type="button" cannot be overridden in CloseModalIcon - Moved type="button" after {...props} spread - Prevents consumer-provided type from overriding (e.g., submit in forms) - Maintains previous behavior as a non-submitting close button All changes verified with successful build. 🤖 Generated with [Claude Code](https://claude.com/claude-code) snaps Filter styled-components theme prop from DOM spreading Addresses Copilot review comment about styled-components injecting a theme prop that should not be spread to DOM elements. Changes: - Added PropsWithPossibleTheme helper type to document the pattern - Filter out theme prop before spreading ...props to DOM elements - Applied consistently across all Modal components: - ModalWrapper, CardWrapper, Card, Header, Heading, BodyHeading, Body, Mask, Footer, CloseModalIcon - Use proper type assertion (not 'as any') to satisfy linting This prevents React warnings about invalid DOM attributes and ensures clean HTML output without theme="[object Object]" attributes. Also added comprehensive documentation to PLAIN_CSS_MIGRATION_LEARNINGS.md explaining: - The issue and why it occurs - Warning signs to watch for - The resolution pattern - Examples for both regular and forwardRef components 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address Copilot review comments: improve code quality and documentation - Fix CSS comment references to point to correct Typography source files (Headings.legacy.ts instead of Typography.legacy.ts) - Update Modal.tsx header comment from 'CSS modules' to 'plain/global CSS' to accurately describe the implementation - Remove unnecessary type casting in theme prop filtering by destructuring theme directly from props parameter, maintaining strong typing throughout - All components now use direct destructuring instead of casting to Record<string, unknown> These changes improve code clarity, maintainability, and type safety while maintaining the same runtime behavior and backward compatibility. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address Copilot review comments: Fix CSS fallbacks and normalize classNames - Updated --header-border fallback from #d5d5d5 to #fafafa (matches theme.color.neutral.darker) - Updated --icon-color-lighter fallback from #6f6f6f to #c5c5c5 (matches theme.color.primary.gray.lighter) - Updated --icon-color-base fallback from #424242 to #5e6062 (matches theme.color.primary.gray.base) These changes ensure the CSS fallback values match the actual theme values that are bound as CSS variables, preventing incorrect styling if CSS variables aren't provided. - Added normalizeClassName helper to remove whitespace-only className values - Applied normalization to all Modal components before passing to classNames() - Prevents trailing/empty classes like `class="modal-body "` from legacy styled-components wrappers - This will be self-resolving once legacy styled-components are removed in future phases All changes verified with successful build. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Remove normalizeClassName/finalizeClassName helpers and document trailing space test artifact The trailing space issue in test snapshots is not a runtime problem but a test artifact caused by Jest's snapshot serialization process stripping out styled-components class names. Changes: - Removed normalizeClassName() helper function from Modal.tsx - Removed finalizeClassName() helper function from Modal.tsx - Simplified all component className assignments to use classNames() directly - Added comprehensive documentation in Modal.spec.tsx explaining the test artifact - Updated styles.legacy.tsx to remove reference to the removed helper Explanation of the test artifact: - Legacy styled() wrappers generate class names like "modal-body styleslegacy__Body-m93cxn-6" - Jest's snapshot serialization strips the styled-components generated classes - This leaves "modal-body " with a trailing space in snapshots only - At runtime, browsers receive the full className with no trailing space issue - This artifact will resolve when legacy wrappers are removed in future phases Addresses review comment from RoyEJohnson in review #16 (PRR_kwDOCVMVFM7vdUKY). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix trailing space issue in Modal className attributes Added finalizeClassName() helper to trim the output of classNames() library, which can produce trailing spaces when some arguments are empty strings. This addresses Copilot's review comment about trailing spaces appearing in snapshots (e.g., "modal-body ", "modal-body-heading ", "modal-footer "). The fix: 1. Enhanced normalizeClassName() to be more explicit about trimming 2. Added finalizeClassName() helper that trims the final computed className 3. Applied finalizeClassName() to all Modal components to ensure clean output This ensures the DOM never receives trailing-space class attributes while maintaining backward compatibility with legacy styled-components wrappers. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Use useIntl instead of FormatMessage to guarantee string Fix Modal component accessibility and TypeScript issues - Add React.forwardRef to CloseModalIcon to accept refs - Explicitly accept and render children in Heading and BodyHeading components - Fix aria-label type by casting to string - Break long lines to meet max-len linting requirements This addresses the code review feedback to fix TypeScript compilation errors and jsx-a11y/heading-has-content warnings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) snaps Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Configure stylelint to lint both styled-components and plain CSS files Added dual stylelint configuration to support the hybrid migration approach where styled-components (.tsx) and plain CSS (.css) coexist during the transition period. Changes: - Created .stylelintrc.css.json for plain CSS files - Uses stylelint-config-standard without styled-components processor - Maintains same linting rules as .stylelintrc for consistency - Updated package.json scripts: - lint:css now runs both lint:css:tsx and lint:css:plain - lint:css:tsx: Lints .tsx files with styled-components (existing) - lint:css:plain: Lints .css files with new config This addresses Copilot's review comment about Modal.css not being covered by CI stylelint. Now both legacy styled-components and new plain CSS files will be linted during the build process. Eventually, when all styled-components are migrated to plain CSS, we can simplify to a single stylelint configuration for .css files only. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix remaining CSS lint errors across the codebase Addressed all remaining stylelint errors and warnings identified in review #21: 1. src/index.css: - Moved @import statements to top of file (lines 1-2) to fix no-invalid-position-at-import-rule errors 2. src/app/components/Checkbox.css: - Added stylelint-disable comment for intentional duplicate outline (webkit fallback) - Added stylelint-disable block for no-descending-specificity warnings (focus-within selectors intentionally override input state selectors for proper CSS cascade) 3. src/app/components/ScrollLock.css: - Added stylelint-disable comment for !important on overflow (must override scroll lock for printing) 4. src/app/content/components/ContentPane.css: - Added stylelint-disable comments for !important on padding-left (must override responsive padding when sidebar is closed) - Added stylelint-disable block for BEM modifier class names with double dashes 5. src/app/components/Typography/Links.css: - Added stylelint-disable block for BEM modifier class names with double dashes All lint disables include explanatory comments. These intentional overrides are necessary for proper functionality (focus states, print styles, responsive behavior, BEM naming convention). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix (most) css lint issues Fix stylelint errors in Modal.css Fixed all stylelint errors identified in lint.output: - Added empty lines before comments (comment-empty-line-before) - Converted hex colors to lowercase (#027EB5 → #027eb5, #0064A0 → #0064a0) - Removed trailing zeros from rem values (3.0rem → 3rem) All changes are formatting/style-only and don't affect functionality. 🤖 Generated with [Claude Code](https://claude.com/claude-code) accommodate BEM patterns Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Address final Copilot review comments (Review #31) 1. Fixed quote consistency in Modal/index.tsx - Changed formatMessage parameter from double quotes to single quotes - Maintains consistency with rest of file 2. Removed unused --link-hover CSS variable - Modal.css only references --link-hover-color - No Typography components use --link-hover - Keeps JS/CSS contract minimal and clear 3. Added explanatory comment for @import positioning in index.css - Clarifies that external fonts complement local @font-face declarations - IBM Plex Mono (external) is for code/monospace usage - Neue Helvetica (local @font-face) is for body text - No conflicts or double-loading 🤖 Generated with [Claude Code](https://claude.com/claude-code) Address final Copilot review comments - Fix Checkbox.css: Change 'Highlight' to 'highlight' for stylelint compliance CSS keywords are case-insensitive, using lowercase for consistency - Fix Modal.tsx: Add --link-hover variable alongside --link-hover-color Ensures compatibility with both Modal.css and Typography link components that use different variable names for hover colors 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint for Button.css (added from main after 2.1 merged) Update index.css Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: staxly[bot] <35789409+staxly[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
It is not used and the Git sha is more descriptive