Phase 2.3: Form Elements Migration#2833
Merged
RoyEJohnson merged 18 commits intomainfrom Apr 14, 2026
Merged
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
RoyEJohnson
force-pushed
the
phase-2.3-form-elements-migration
branch
from
58cdf19 to
1882365
Compare
RoyEJohnson
force-pushed
the
phase-2.3-form-elements-migration
branch
from
1882365 to
af81407
Compare
RoyEJohnson
force-pushed
the
phase-2.3-form-elements-migration
branch
from
af81407 to
abd2f18
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
OpenStaxClaude
added a commit
that referenced
this pull request
Mar 29, 2026
Moved ContentPane.css import to global CSS loader in app/index.tsx to resolve webpack chunk ordering conflict with Note.css. The build was failing with: "Conflicting order. Following module has been added: * css ...ContentPane.css despite it was not able to fulfill desired ordering with these modules: * css ...Note.css" This follows the established pattern of importing component CSS files in app/index.tsx to ensure consistent chunk ordering across routes. Fixes: Review comment from RoyEJohnson in PR #2833 🤖 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.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
OpenStaxClaude
added a commit
that referenced
this pull request
Mar 30, 2026
Updated the global button border reset to use :not([class*="button"]) selector, which preserves borders on styled button components (like .button-default, .button-primary) while still removing unwanted default borders from unstyled buttons. This fixes the issue where the Cancel button in EditCard (which uses the Button component with variant="default") was losing its border due to the global reset overriding .button-default styles. The :not([class*="button"]) approach is automatic and doesn't require adding special classes to preserve borders - any button with a class name containing "button" will keep its intended styling. Addresses PR #2833, Review 7. 🤖 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.
RoyEJohnson
force-pushed
the
phase-2.3-form-elements-migration
branch
from
692579c to
c6bed21
Compare
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 31, 2026
Fixed two issues identified in the latest code review:
1. Button.css selector specificity (Comment 1):
- Changed `button.plain-button` and `button.button-link` to just
`.plain-button` and `.button-link`
- Removes element selector to reduce specificity
- Allows styled-components wrappers (e.g., NudgeCloseButton) to
override with their own border declarations
- Matches the old behavior where component-specific styles could
override the reset
2. Note.spec.tsx test cleanup (Comment 2):
- Added empty dependency array `[]` to useLayoutEffect
- Added cleanup function that removes the event listener
- Captures div ref in local variable for cleanup closure
- Prevents multiple listener registrations on re-renders
- Eliminates potential test flakiness
Both changes improve code quality and follow React best practices.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix CSS loading order conflict with ContentPane.css
Moved ContentPane.css import to global CSS loader in app/index.tsx
to resolve webpack chunk ordering conflict with Note.css.
The build was failing with:
"Conflicting order. Following module has been added:
* css ...ContentPane.css
despite it was not able to fulfill desired ordering with these modules:
* css ...Note.css"
This follows the established pattern of importing component CSS files
in app/index.tsx to ensure consistent chunk ordering across routes.
Fixes: Review comment from RoyEJohnson in PR #2833
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix ESC handler bug and update PR description
Addressed latest Copilot review comments:
1. Fixed ESC handler bug in Note.tsx (Comment 1):
- Changed condition from checking textareaRef.current?.textContent
to checking the note prop directly
- For <textarea> elements, user-entered content is in .value property,
not textContent (which would always be empty)
- This prevents ESC from incorrectly dispatching hideCardEvent when
the textarea has content
- Added note to dependency array of useCallback
2. Updated PR description (Comment 2):
- Clarified that Answer.css was initially created but then deleted
during first code review
- Explained we now use the standard .hidden-but-accessible utility
instead for better consistency and screen reader support
- Added note section explaining the change
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix AuthenticationGate button
Refine button border reset to exclude styled buttons
Updated the global button border reset to use :not([class*="button"])
selector, which preserves borders on styled button components (like
.button-default, .button-primary) while still removing unwanted default
borders from unstyled buttons.
This fixes the issue where the Cancel button in EditCard (which uses
the Button component with variant="default") was losing its border due
to the global reset overriding .button-default styles.
The :not([class*="button"]) approach is automatic and doesn't require
adding special classes to preserve borders - any button with a class
name containing "button" will keep its intended styling.
Addresses PR #2833, Review 7.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Address Copilot Review comments on button border handling
Fixed two issues identified in the latest Copilot review:
1. Button.css - Added explicit border reset for PlainButton classes:
- Added `button.plain-button` and `button.button-link` to explicitly
reset borders on these base button classes
- These classes contain "button" in their names, so they're excluded
from the `button:not([class*="button"])` selector
- Without this explicit reset, they could inherit browser default borders
- This ensures consistent border behavior across all button types
2. AuthenticationGate.tsx - Removed undefined basic-button class:
- Removed `className="basic-button"` which had no corresponding CSS definition
- This was added mistakenly in an earlier commit to work around border issues
- The button now correctly matches `button:not([class*="button"])` and
gets the border reset as intended
- No visual change since basic-button had no styles anyway
Both changes ensure proper border handling without introducing undefined
classes or leaving gaps in the reset selector coverage.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Update Button.css
Remove box-shadow from disabled button-primary state
Fixed Review 11 comment: Disabled buttons should not have a box-shadow.
The base .button class applies a box-shadow, but disabled buttons should
not display this shadow. Updated both the main disabled state rule and
the hover/active/focus override rules to set box-shadow: none.
This ensures all disabled buttons (including button-primary) are rendered
without shadows, making them visually distinct from enabled buttons.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Note test coverage
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
force-pushed
the
phase-2.3-form-elements-migration
branch
from
c6bed21 to
40afc15
Compare
RoyEJohnson
pushed a commit
that referenced
this pull request
Mar 31, 2026
Fixed two issues identified in the latest code review:
1. Button.css selector specificity (Comment 1):
- Changed `button.plain-button` and `button.button-link` to just
`.plain-button` and `.button-link`
- Removes element selector to reduce specificity
- Allows styled-components wrappers (e.g., NudgeCloseButton) to
override with their own border declarations
- Matches the old behavior where component-specific styles could
override the reset
2. Note.spec.tsx test cleanup (Comment 2):
- Added empty dependency array `[]` to useLayoutEffect
- Added cleanup function that removes the event listener
- Captures div ref in local variable for cleanup closure
- Prevents multiple listener registrations on re-renders
- Eliminates potential test flakiness
Both changes improve code quality and follow React best practices.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix CSS loading order conflict with ContentPane.css
Moved ContentPane.css import to global CSS loader in app/index.tsx
to resolve webpack chunk ordering conflict with Note.css.
The build was failing with:
"Conflicting order. Following module has been added:
* css ...ContentPane.css
despite it was not able to fulfill desired ordering with these modules:
* css ...Note.css"
This follows the established pattern of importing component CSS files
in app/index.tsx to ensure consistent chunk ordering across routes.
Fixes: Review comment from RoyEJohnson in PR #2833
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix ESC handler bug and update PR description
Addressed latest Copilot review comments:
1. Fixed ESC handler bug in Note.tsx (Comment 1):
- Changed condition from checking textareaRef.current?.textContent
to checking the note prop directly
- For <textarea> elements, user-entered content is in .value property,
not textContent (which would always be empty)
- This prevents ESC from incorrectly dispatching hideCardEvent when
the textarea has content
- Added note to dependency array of useCallback
2. Updated PR description (Comment 2):
- Clarified that Answer.css was initially created but then deleted
during first code review
- Explained we now use the standard .hidden-but-accessible utility
instead for better consistency and screen reader support
- Added note section explaining the change
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix AuthenticationGate button
Refine button border reset to exclude styled buttons
Updated the global button border reset to use :not([class*="button"])
selector, which preserves borders on styled button components (like
.button-default, .button-primary) while still removing unwanted default
borders from unstyled buttons.
This fixes the issue where the Cancel button in EditCard (which uses
the Button component with variant="default") was losing its border due
to the global reset overriding .button-default styles.
The :not([class*="button"]) approach is automatic and doesn't require
adding special classes to preserve borders - any button with a class
name containing "button" will keep its intended styling.
Addresses PR #2833, Review 7.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Address Copilot Review comments on button border handling
Fixed two issues identified in the latest Copilot review:
1. Button.css - Added explicit border reset for PlainButton classes:
- Added `button.plain-button` and `button.button-link` to explicitly
reset borders on these base button classes
- These classes contain "button" in their names, so they're excluded
from the `button:not([class*="button"])` selector
- Without this explicit reset, they could inherit browser default borders
- This ensures consistent border behavior across all button types
2. AuthenticationGate.tsx - Removed undefined basic-button class:
- Removed `className="basic-button"` which had no corresponding CSS definition
- This was added mistakenly in an earlier commit to work around border issues
- The button now correctly matches `button:not([class*="button"])` and
gets the border reset as intended
- No visual change since basic-button had no styles anyway
Both changes ensure proper border handling without introducing undefined
classes or leaving gaps in the reset selector coverage.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Update Button.css
Remove box-shadow from disabled button-primary state
Fixed Review 11 comment: Disabled buttons should not have a box-shadow.
The base .button class applies a box-shadow, but disabled buttons should
not display this shadow. Updated both the main disabled state rule and
the hover/active/focus override rules to set box-shadow: none.
This ensures all disabled buttons (including button-primary) are rendered
without shadows, making them visually distinct from enabled buttons.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Note test coverage
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Migrated TextArea, SimpleLabel, and AnswerInput components following the established patterns from Phase 1.1-2.1. Changes: - Created Note.css for TextArea and SimpleLabel styles - Updated Note.tsx to use plain HTML elements with CSS variables - Created Answer.css for AnswerInput styles - Updated Answer/index.tsx to use plain input element - Removed styled-components from these form elements All styling and behavior should remain identical. This is a refactoring-only change with no visual differences. Related to CORE-1697 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed three issues identified in code review: 1. Note.css duplicate color property (Comments 1-2): - Removed redundant --text-color and --label-color declarations - Consolidated to single --note-text-color variable for clarity - Updated Note.tsx to set only --note-text-color 2. Answer.css unnecessary duplication (Comment 3): - Removed Answer.css file entirely - Changed answerInputClass to re-export hiddenButAccessibleClass - Now uses standard .hidden-but-accessible utility from index.css - Provides better screen reader consistency with clip/overflow rules 3. CSS import location (Comment 4): - Removed Answer.css import (file deleted, see #2) - Added hiddenButAccessibleClass import to styled.tsx All form elements now follow established patterns and use standard utilities where appropriate, improving maintainability. 🤖 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>
Fixed two issues identified in the latest code review:
1. Button.css selector specificity (Comment 1):
- Changed `button.plain-button` and `button.button-link` to just
`.plain-button` and `.button-link`
- Removes element selector to reduce specificity
- Allows styled-components wrappers (e.g., NudgeCloseButton) to
override with their own border declarations
- Matches the old behavior where component-specific styles could
override the reset
2. Note.spec.tsx test cleanup (Comment 2):
- Added empty dependency array `[]` to useLayoutEffect
- Added cleanup function that removes the event listener
- Captures div ref in local variable for cleanup closure
- Prevents multiple listener registrations on re-renders
- Eliminates potential test flakiness
Both changes improve code quality and follow React best practices.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix CSS loading order conflict with ContentPane.css
Moved ContentPane.css import to global CSS loader in app/index.tsx
to resolve webpack chunk ordering conflict with Note.css.
The build was failing with:
"Conflicting order. Following module has been added:
* css ...ContentPane.css
despite it was not able to fulfill desired ordering with these modules:
* css ...Note.css"
This follows the established pattern of importing component CSS files
in app/index.tsx to ensure consistent chunk ordering across routes.
Fixes: Review comment from RoyEJohnson in PR #2833
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix ESC handler bug and update PR description
Addressed latest Copilot review comments:
1. Fixed ESC handler bug in Note.tsx (Comment 1):
- Changed condition from checking textareaRef.current?.textContent
to checking the note prop directly
- For <textarea> elements, user-entered content is in .value property,
not textContent (which would always be empty)
- This prevents ESC from incorrectly dispatching hideCardEvent when
the textarea has content
- Added note to dependency array of useCallback
2. Updated PR description (Comment 2):
- Clarified that Answer.css was initially created but then deleted
during first code review
- Explained we now use the standard .hidden-but-accessible utility
instead for better consistency and screen reader support
- Added note section explaining the change
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Fix AuthenticationGate button
Refine button border reset to exclude styled buttons
Updated the global button border reset to use :not([class*="button"])
selector, which preserves borders on styled button components (like
.button-default, .button-primary) while still removing unwanted default
borders from unstyled buttons.
This fixes the issue where the Cancel button in EditCard (which uses
the Button component with variant="default") was losing its border due
to the global reset overriding .button-default styles.
The :not([class*="button"]) approach is automatic and doesn't require
adding special classes to preserve borders - any button with a class
name containing "button" will keep its intended styling.
Addresses PR #2833, Review 7.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Address Copilot Review comments on button border handling
Fixed two issues identified in the latest Copilot review:
1. Button.css - Added explicit border reset for PlainButton classes:
- Added `button.plain-button` and `button.button-link` to explicitly
reset borders on these base button classes
- These classes contain "button" in their names, so they're excluded
from the `button:not([class*="button"])` selector
- Without this explicit reset, they could inherit browser default borders
- This ensures consistent border behavior across all button types
2. AuthenticationGate.tsx - Removed undefined basic-button class:
- Removed `className="basic-button"` which had no corresponding CSS definition
- This was added mistakenly in an earlier commit to work around border issues
- The button now correctly matches `button:not([class*="button"])` and
gets the border reset as intended
- No visual change since basic-button had no styles anyway
Both changes ensure proper border handling without introducing undefined
classes or leaving gaps in the reset selector coverage.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Update Button.css
Remove box-shadow from disabled button-primary state
Fixed Review 11 comment: Disabled buttons should not have a box-shadow.
The base .button class applies a box-shadow, but disabled buttons should
not display this shadow. Updated both the main disabled state rule and
the hover/active/focus override rules to set box-shadow: none.
This ensures all disabled buttons (including button-primary) are rendered
without shadows, making them visually distinct from enabled buttons.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Note test coverage
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The SkipAndSubmitButtons were not receiving the margin-left: 0.1rem
styling specified in the QuestionNavigation Wrapper component selector.
Root Cause:
- QuestionNavigation's Wrapper uses styled-components selector ${Button}
to apply margin-left to buttons (line 28-30)
- SkipAndSubmitButtons was importing the plain Button component directly
- After Button migration to plain CSS, component selectors only work with
styled-component wrappers
Fix:
- Exported the styled Button wrapper from QuestionNavigation/index.tsx
- Updated SkipAndSubmitButtons to import the styled Button instead of
the plain Button component
- Now buttons in SkipAndSubmitButtons match the ${Button} selector and
receive the margin-left styling
This ensures proper button spacing in the practice questions navigation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Fixed two issues identified in the latest code review:
1. Fixed :empty CSS selector issue in Note.css (Comment 1):
- Issue: The :empty pseudo-class doesn't detect textarea value changes
(it only checks for child nodes, not the value property)
- Fix: Replaced .note-textarea:empty with data-attribute selector
- Added data-empty={note === '' ? 'true' : 'false'} to textarea element
- Now padding-top styling correctly applies based on actual note content
2. Fixed circular dependency in QuestionNavigation (Comment 2):
- Issue: SkipAndSubmitButtons imported { Button } from './index',
but index.tsx also imports SkipAndSubmitButtons, creating circular dependency
- Fix: Extracted styled Button wrapper to separate Button.tsx file
- Both index.tsx and SkipAndSubmitButtons.tsx now import from Button.tsx
- Eliminates circular dependency while preserving styled-component selector behavior
Both fixes maintain existing functionality while following React and module best practices.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
This was leftover behavior for when the label was embedded in the textarea. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> Co-Authored-By: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
jivey
approved these changes
Apr 14, 2026
RoyEJohnson
approved these changes
Apr 14, 2026
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
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.
CORE-1697
Summary
Migrates form element components from styled-components to plain CSS, following the established patterns from Phase 1.1-2.1.
Components Migrated
Changes
Note.csswith styles for TextArea and SimpleLabelNote.tsxto use plain HTML elements with CSS variablesAnswer/styled.tsxto re-export the standardhiddenButAccessibleClassutility asanswerInputClassAnswer/index.tsxto use plain<input>element with classNameVisible change: There was a dynamic bit of padding in the highlighter note textarea when the text was empty. It was there to make room for when the field label was rendered inside the textarea, which we don't do anymore. It caused the textarea size to shift slightly when going from empty to not or vice-versa. Now it doesn't.
Note: Originally created
Answer.css, but during code review it was identified that the codebase already has a standard.hidden-but-accessibleutility class. The Answer.css file was removed in favor of using the existing utility for better consistency and screen reader support.Migration Approach
These components are internal (not exported) and not used in component selectors, so we used a simple in-place migration without the full hybrid approach. No legacy exports needed.
Testing Notes
This is a refactoring-only change with no visual differences. All styling, layout, and behavior should remain identical:
Test Plan
Manual Testing
Highlights Note Textarea
Practice Questions Answer Inputs
Automated Testing
CI will run all existing tests to ensure no regressions.
Related
🤖 Generated with Claude Code