Phase 2.6: Content Page Components Migration#2926
Conversation
Address all 6 review comments from PR #2926: 1. Replace `any` casts with `unknown` - Changed all `props as any` to `props as unknown as Record<string, unknown>` in Toolbar/styled.tsx and Topbar/styled.tsx for better type safety. 2. Add missing `shadow` export - Exported the `shadow` CSS template literal in Topbar/styled.tsx that is imported by AssignedTopBar.tsx. 3. Fix MainContent `id` prop - Removed redundant `id={MAIN_CONTENT_ID}` prop from PageContent.tsx as MainContent already sets this internally on ContentStyles component. 4. Fix `colorSchema` type mismatch - Added explicit type annotation `BookWithOSWebData['theme'] | null` to searchButtonColor variable in Topbar/index.tsx to ensure type compatibility. 5. Fix TextResizerChangeButton `onClick` type - Changed onClick event handlers from `React.FormEvent<HTMLInputElement>` to `React.MouseEvent<HTMLButtonElement>` since they're attached to buttons. 6. Fix SidebarSearchInput `colorSchema` type - Updated ResultsSidebarProps interface to use `BookWithOSWebData['theme'] | null` instead of `string | null` for searchButtonColor property. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
9d73de9 to
01ca470
Compare
This comment was marked as resolved.
This comment was marked as resolved.
ca2d3aa to
3af3902
Compare
ed17a88 to
6cc8ac6
Compare
e216d4e to
98c8cff
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Migrated three critical content page layout components from styled-components to plain CSS following the established hybrid approach (theme in JavaScript, styles in CSS). Components migrated: - PageContent (src/app/content/components/Page/) - Topbar (src/app/content/components/Topbar/) - Toolbar (src/app/content/components/Toolbar/) Files created: - PageContent.css - Static styles for page content container - PageContent.legacy.ts - Legacy export for contentTextStyle - Topbar.css - All topbar static styles - Toolbar.css - All toolbar styles including mobile menu animations - Toolbar.legacy.ts - Legacy exports for CSS fragments Key changes: - Converted 25+ styled components to plain React components with CSS classes - Created CSS files with all static styles and top-level media queries - Used CSS variables bound from JavaScript for dynamic theme values - Maintained icon components wrapped with styled() for component selector compatibility - Created .legacy.ts files for backward compatibility with existing imports - Implemented theme prop filtering to prevent DOM attribute leakage - Used classNames library for conditional class composition - Preserved all existing functionality and component APIs Technical details: - PageContent: Dynamic highlight styles generated from constants array - Topbar: Complex responsive behavior with book theme colors and text resizer gradient - Toolbar: Mobile menu animations with keyframes Migration patterns followed: - Hybrid approach (theme in JS, styles in CSS) - CSS variables for dynamic values - Top-level media queries (not nested) - Theme prop filtering before spreading to DOM - React.forwardRef for ref forwarding - Backward-compatible legacy exports - Icon components wrapped with styled() for compatibility Related: CORE-1700 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.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>
Fixed 11 unresolved Copilot review issues: 1. **CSS variable inheritance in MainContent** - Moved style prop from inner ContentStyles to outer <main> element so CSS variables inherit correctly down to .page-content class. 2. **Print layout fix** - Moved max-width and margin rules into @media screen block to prevent width constraints in print layouts. 3. **Filter non-DOM props in PlainButton** - Added explicit filtering of `isActive` and `practiceQuestionsEnabled` props to prevent React warnings about unknown DOM attributes. 4. **SearchResultsTextButton typography** - Added missing textRegularStyle and decoratedLinkStyle rules (font-size: 1.6rem, line-height: 2.5rem, link colors, hover states) to Topbar.css. 5. **SearchButton icon color** - Changed from theme.color.primary[].base to theme.color.primary[].foreground for better contrast when button background is colored. 6. **SearchButton duplicate aria-label** - Fixed logic to use ariaLabelId when provided, otherwise fall back to default label (removes duplicate aria-label declarations). 7. **SearchInputWrapper icon color** - Same fix as SearchButton, using foreground color for proper contrast. 8. **search-in-sidebar display logic** - Removed unconditional display:none rule, now only hidden at mobile-medium breakpoint and above when searchInSidebar is true. All changes improve accessibility, visual consistency, and prevent React/DOM warnings. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Topbar form right-justified on mobile Display TextResizer on mobile Update Topbar.css Times icon sizing Print button layout Some Copilot issues Typo fixed 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: 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>
Fix all 9 unresolved inline comments from Copilot reviews: 1. **useMemo dependency array (PageContent.tsx)** - Added eslint-disable comment for exhaustive-deps rule since highlightStyles and theme are module-scoped constants that don't need to be in the dependency array. 2. **SVG focusable attribute (Toolbar/styled.tsx)** - Added `focusable="false"` to TimesIcon component to prevent keyboard focus in some browsers/assistive technology combinations. 3. **SVG focusable attribute (Topbar/styled.tsx)** - Added `focusable="false"` to CloseIcon component for accessibility consistency. 4. **Remove unused CSS (Toolbar.css)** - Removed `.toolbar-left-arrow` styles since LeftArrow component is still implemented as a styled-component and doesn't use this class. 5. **Remove unused CSS (Topbar.css)** - Removed `.topbar-left-arrow` styles since LeftArrow component doesn't apply this class. 6. **Filter non-DOM props (Topbar/styled.tsx)** - Fixed SearchPrintWrapper to filter out `dispatch`, `isDesktopSearchOpen`, and `isTocOpen` props injected by isVerticalNavOpenConnector HOC before spreading to DOM element. This prevents React unknown-prop warnings. Notes: - Comment 1 (PageContent.css print layout) was already addressed - Comments 5-6 about SearchButton aria-label were already fixed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Move dynamicHighlightStyles out of PageContent function Update Toolbar.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: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Update CSS selector in AltSCycler to use plain CSS class name instead of styled-components generated class pattern. Changed selector from `[class*="SearchInputWrapper"] input` to `.topbar-search-input-wrapper input` to match the new plain CSS class name applied after migrating from styled-components. This fixes the failing tests in Topbar/index.spec.tsx: - "goes to main when no search results" - "goes to search results when provided" - "aborts on mobile" These tests verify that Alt+S keyboard shortcut correctly cycles focus between search input, search results, and main content. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.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>
1. Add test coverage for hasSearchResults branch in AltSCycler
- Added test: cycles between search input and main when no search
results and focus is in search input
- Added test: cycles between main and search input when no search
results and focus is in main
- These tests cover the else branch at line 212-215 which toggles
between search input (index 0) and main content (index 2) when
there are no search results and focus is already in one of the regions
2. Remove unused applySearchIconColor function
- Deleted src/app/content/components/utils/applySearchIconColor.ts
- This function was replaced by inline logic during the CSS migration
and is no longer used anywhere in the codebase
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Lint stuff
Test stuff
Remove invalid value attribute from SearchButton
The value attribute is not valid for button elements. The aria-label
already provides the accessible name using the ariaLabelId when provided,
or falling back to the default search label.
Addresses Copilot review comment 3/3.
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Addresses Review 15 feedback to improve test coverage for the closedStyle branch in sidebar.ts line 27. Created comprehensive tests for styleWhenSidebarClosed and styleWhenTocClosed functions with explicit assertions that verify: - The interpolation functions execute with the right props - The closedStyle variable is actually used/returned - Both branches of the && operator are exercised Tests explicitly assert fnResult.toBe(closedStyle) and fnResult.toEqual(closedStyle) to ensure coverage tools properly detect branch coverage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix SearchPrintWrapper sidebar-closed logic Address Copilot review comment 4/4 (Review 23). **Issue:** SearchPrintWrapper was incorrectly applying `sidebar-closed` class when `!isVerticalNavOpen`, but `isVerticalNavOpen` can be `null` (meaning "open on desktop / closed on mobile"). This caused `!null` to evaluate to `true`, incorrectly applying the closed-sidebar padding reset on desktop layouts. Additionally, `isDesktopSearchOpen` was being filtered out but not used in the logic. **Fix:** - Changed condition from `!isVerticalNavOpen` to the correct logic: - `sidebar-closed`: Applied only when BOTH `isDesktopSearchOpen === false` AND `isVerticalNavOpen === false` (desktop closed state) - `sidebar-mobile-closed`: Applied when `isVerticalNavOpen === null` (mobile closed state) - Added corresponding CSS rule for `sidebar-mobile-closed` class scoped to mobile breakpoint - Preserves the original `styleWhenSidebarClosed` semantics from the styled-components implementation This ensures proper padding behavior across all sidebar states: - Desktop + both closed: padding-left: 0 - Desktop + nav open: normal padding - Mobile + nav null: padding-left: 0 - Mobile medium: padding via media query 🤖 Generated with [Claude Code](https://claude.com/claude-code) Cleanup Copilot - fix specificity Remove non-working cycler tests Update index.spec.tsx Fix unresolved code review comments Address the remaining unresolved inline comments from Copilot review: 1. **Topbar test selectors**: Replace styled-components class name selectors `[class*="TopBar"]` with `[data-testid="topbar"]` in all 5 test cases. The component now uses plain CSS and the old class name selector would return null, causing tests to fail. 2. **Search icon hover/focus**: Add CSS rules to preserve the themed search icon color on hover/focus states. Without this, the generic toolbar-plain-button hover rule would override the themed icon color, making themed/white icons turn dark unexpectedly. Files changed: - src/app/content/components/Topbar/index.spec.tsx: Updated 5 test selectors - src/app/content/components/Topbar/Topbar.css: Added hover/focus color preservation Notes: - Comment 1 (PageContent.css max-width) was already fixed in previous commits - Comment 3 (SearchButton icon color) was already fixed in previous commits - Comment 4 (aria-label duplication) was already fixed in previous commits - Comment 6 (sidebar.spec.ts brittleness) is a suggestion for future refactoring 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix sidebar utility test failures The previous tests incorrectly assumed the css template literal returned an array with interpolation functions at indices [0] and [1]. However, styled-components css`` returns an array where string fragments are at even indices and interpolation functions are at odd indices. Fixed by: - Accessing interpolation functions at correct indices (1 and 3) - Consolidating tests into comprehensive test cases - Adding explicit assertions to ensure closedStyle is returned when conditions are met (for code coverage) This addresses Review 16 test failures. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix sidebar utility functions to return undefined instead of false Address Review 21 feedback: The interpolation functions in sidebar.ts were using `&&` operators which return `false` when conditions aren't met. This caused test failures expecting `undefined`. Changed from: props.isTocOpen === false && closedStyle To: props.isTocOpen === false ? closedStyle : undefined This ensures the functions return `undefined` (instead of `false`) when conditions aren't met, which is the expected behavior for styled-components interpolation functions. Fixes: - styleWhenTocClosed now returns undefined when isTocOpen is true - styleWhenSidebarClosed now returns undefined when either isDesktopSearchOpen or isVerticalNavOpen is true 🤖 Generated with [Claude Code](https://claude.com/claude-code) Fix sidebar utility test array index assumptions Address Review 18 feedback about sidebar.spec.ts test failures. The previous tests assumed interpolation functions would be at specific array indices (1 and 3) in the styled-components css`` template result. However, when theme.breakpoints.mobile() is called inside an interpolation, it can return its own nested array structure that gets flattened, making the array indices unpredictable. Changes: - Instead of assuming specific indices, now dynamically find interpolation functions by filtering the result array for functions - Test each function by calling it with test props to identify which one is the closed state function we want to verify - Maintains the same test coverage (ensuring closedStyle is returned when conditions are met) without brittle index assumptions - Uses try/catch to gracefully handle functions that don't accept the expected props This approach is more robust and doesn't depend on the internal structure of styled-components template literal arrays. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update sidebar.spec.ts 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: Copilot <175728472+Copilot@users.noreply.github.com>
Fixed 4 critical type safety and code quality issues: 1. Fixed duplicate className in PageContent - Component now checks if 'page-content' class already exists before prepending it, preventing duplicate classes like "page-content page-content" in rendered output. 2. Fixed type casting in Toolbar PlainButton - Replaced unsafe `props as Record<string, unknown>` cast with proper destructuring. Non-DOM props (theme, isActive, practiceQuestionsEnabled) are now filtered via destructuring, preserving type safety for domProps spread. 3. Fixed type casting in Topbar PlainButton - Similarly replaced unsafe type cast with proper destructuring of theme prop, maintaining type safety when spreading to PlainButtonBase. 4. Fixed SearchPrintWrapper prop type - Updated isVerticalNavOpen type from `boolean | undefined` to `boolean | null` to match runtime behavior where selector can return null (when tocOpen is null and search results are closed). These changes improve type safety by avoiding unsafe type casts and ensure components properly handle all possible prop values. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Lint line length 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 Copilot review comment about unnecessary DOM churn. **Issue:** PageContent was creating a new <style> element on every component render/mount, even though the styles are static. This causes unnecessary DOM churn and makes styles harder to debug. **Solution:** - Use a singleton pattern with module-level flag to inject styles only once - Inject styles into document.head via useEffect on first mount - Remove the per-instance <style> tag from the JSX The highlight styles are still computed once at module load time, but now the style element is only created and inserted once globally, regardless of how many PageContent instances are rendered. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update routes.spec.tsx.snap Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Per PLAIN_CSS_MIGRATION_GUIDE.md Pattern 2.5, replace component-level bindings of static theme colors with root-level CSS variables defined in src/index.css. Changes: - Topbar: Replace component-level bindings with root-level variables - theme.color.text.label → --color-text-label - theme.color.neutral.base → --color-neutral-base - theme.color.primary.gray.base → --color-primary-gray-base - theme.color.primary.gray.medium → --color-primary-gray-medium - Toolbar: Replace component-level bindings with root-level variables - theme.color.neutral.darker → --color-neutral-darker - theme.color.neutral.formBorder → --color-neutral-form-border - theme.color.primary.gray.base → --color-primary-gray-base - PageContent: Use root-level variable in generated highlight styles - theme.color.text.white → --color-text-white Benefits: - Reduces code duplication across components - Improves maintainability by centralizing static color definitions - Aligns with established migration patterns - Reduces unnecessary JavaScript-to-CSS variable binding overhead Note: Dynamic colors (book themes, computed values) continue using component-level bindings as per hybrid approach. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Update index.spec.tsx.snap Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
RoyEJohnson
left a comment
There was a problem hiding this comment.
Address comments from Josiah (jivey).
Addresses review comment from jivey (Comment 6/6) - empty CSS rules like serve no purpose and should be removed. The media query rule at line 219 already handles the desktop-specific styling, making the empty placeholder redundant.
Inline Review Comments - Status UpdateI've reviewed all 6 unresolved inline comments from the Copilot and team reviews. Here's the current status: ✅ Comment 1/6: PageContent.css print layout (copilot)Status: Already resolved in previous commits 📝 Comment 2/6: PageContent dynamic highlight styles (jivey)Suggestion: Use CSS variables for highlight colors and break out position styles The current implementation generates CSS dynamically from the
This is a significant refactoring that would be better suited for a dedicated ticket. The current singleton injection pattern (added in commit 97ded67) already optimizes performance by injecting styles once globally rather than per-component-instance. Recommendation: Defer to future optimization ticket ❓ Comment 3/6: Toolbar theme prop type (jivey)Question: Should The
✅ Comment 4/6: SearchButton aria-label (copilot)Status: Already resolved in previous commits aria-label={intl.formatMessage({
id: ariaLabelId || 'i18n:search-results:bar:search-icon:value',
})}No duplication exists - the aria-label is set once using the provided ID when available, otherwise using the default. ✅ Comment 5/6: SearchButton icon color (copilot)Status: Already resolved in previous commits ✅ Comment 6/6: Empty CSS placeholder rule (jivey)Status: Fixed in commit cd28238 SummaryResolved: 4/6 comments (already fixed in previous commits) All critical type safety and code quality issues have been addressed. The one deferred item (Comment 2) is a valid optimization suggestion that would be better tackled in a dedicated refactoring ticket. Latest commit: cd28238 |
Summary
Migrated three critical content page layout components from styled-components to plain CSS as part of Phase 2.6 of the Plain CSS Migration initiative.
Related Jira: CORE-1700
Components Migrated
1. PageContent (
src/app/content/components/Page/)highlightStylesarray in constantsPageContent.cssfor static stylesPageContent.legacy.tsfor backward-compatiblecontentTextStyleexportAttribution.tsxto import from legacy file2. Topbar (
src/app/content/components/Topbar/)Topbar.csswith all responsive media queries3. Toolbar (
src/app/content/components/Toolbar/)Toolbar.csswith animation keyframesToolbar.legacy.tsfor backward-compatible exports (PlainButton,barPadding,toolbarDefaultButton, etc.)HighlightButton.tsx,PracticeQuestionsButton.tsx,StudyGuidesButton.tsx,SidebarControl/Buttons.tsxTechnical Implementation
CSS Variables for Dynamic Theming
Dynamic values (book theme colors, padding, gradients) are bound as CSS variables from JavaScript:
Theme Prop Filtering (NEW Pattern)
Implemented the pattern from the updated PLAIN_CSS_MIGRATION_LEARNINGS.md to filter styled-components internal props (theme) before spreading to DOM elements:
This prevents React warnings about invalid DOM props and keeps the DOM clean.
Icon Components
Kept icon components wrapped with
styled()using empty template literals for component selector compatibility:Backward Compatibility
Created
.legacy.tsfiles to maintain exports used by other components:PageContent.legacy.ts: ExportscontentTextStyle(used by Attribution.tsx)Toolbar.legacy.ts: ExportsPlainButton,barPadding,toolbarDefaultButton, etc. (used by Topbar, Toolbar buttons, SidebarControl)Responsive Design
All media queries moved to top level in CSS (required for plain CSS):
Migration Patterns Followed
✅ Hybrid approach (theme in JavaScript, styles in CSS)
✅ CSS variables bound from theme at component level
✅ Top-level media queries (not nested)
✅ Separate
.legacy.tsfiles for backward compatibility✅
classNameslibrary for conditional class composition✅
React.forwardReffor components receiving refs✅ Theme prop filtering (new pattern from LEARNINGS file)
✅ Icon components wrapped with
styled()for compatibility✅ CSS variables set BEFORE
...stylespreadFiles Changed
Testing Requirements
Responsive Testing
Test at all three breakpoints:
Topbar Functionality
Toolbar Functionality
PageContent / Highlights
Z-Index and Layering
Visual Regression
🤖 Generated with Claude Code
Co-Authored-By: Claude Sonnet 4.5 noreply@anthropic.com