Skip to content

fix: improve Input component alignment and sizing - #4531

Merged
millus merged 2 commits into
mainfrom
fix/input-adjustments-4522
Feb 20, 2026
Merged

fix: improve Input component alignment and sizing#4531
millus merged 2 commits into
mainfrom
fix/input-adjustments-4522

Conversation

@millus

@millus millus commented Feb 18, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes #4522 - Improves Input component
button alignment, sizing, density
handling, and token usage.

Key Changes

Button Alignment & Sizing:

  • Constrain buttons in adornments to
    20x20px to match Input content height
  • Add calculated negative margins to
    align buttons with icon adornments
  • Remove data-font-family from adornment
    wrappers to allow margins to work
  • Use design tokens for sizing and
    margins

Density Support:

  • Fix comfortable density height to 24px
    (was 28px)
  • Use
    --eds-spacing-proportions-xs-vertical
    token for comfortable override
  • Spacious remains at 36px

Semantic Token Aliases:

  • Add --_bg-input alias for input
    background (until proper token
    available)
  • Add --_placeholder-text-color alias on
    .eds-input element (ensures gray in
    invalid state)
  • Add --_readonly-text-color alias for
    readonly text
  • Replace --eds-color-border-medium with
    --eds-color-text-disabled (aligned with
    PR refactor: use semantic disabled tokens in next components #4529)

API Improvements:

  • Rename showErrorIcon → hideErrorIcon
    (more intuitive - only set when hiding)
  • Update all components, tests, and
    documentation

TextField Storybook Improvements:

  • Add hideErrorIcon and
    containerClassName controls
  • Reorganize controls into clear
    categories (States, Styling, Other)
  • Remove outdated color behavior
    descriptions from adornments

Other Improvements:

  • Use modern logical properties
    (margin-inline-start/end)
  • Add Safari autofill override to
    default placeholder
  • Update stories with Search patterns
    and European currency examples
  • Clean up documentation and comments

Visual Changes

  • Input height: Spacious 36px ✓,
    Comfortable 24px ✓
  • Buttons in adornments now properly
    aligned with icons
  • Placeholder stays gray in invalid
    state (background/border turn red)
  • Adornment colors stay neutral in all
    states

Test Plan

  • ✅ All 42 Input tests pass
  • ✅ Tests updated for hideErrorIcon
    rename
  • ✅ Manual testing in Storybook for
    both densities
  • ✅ Search pattern examples working
    correctly
  • ✅ Placeholder color correct in all
    states

🤖 Generated with
https://claude.com/claude-code

@millus millus changed the title fix(next): improve Input component alignment and sizing fix: improve Input component alignment and sizing Feb 18, 2026
@millus
millus force-pushed the fix/input-adjustments-4522 branch 9 times, most recently from 6a3fda2 to 47b80a7 Compare February 18, 2026 10:25
@pomfrida

Copy link
Copy Markdown
Collaborator

@claude review

@claude

claude Bot commented Feb 18, 2026

Copy link
Copy Markdown

Claude finished @pomfrida's task in 1m 27s —— View job


Code Review Complete

  • Read and analyze all changed files
  • Review code quality and EDS conventions
  • Check accessibility compliance
  • Review API changes (showErrorIcon → hideErrorIcon)
  • Provide comprehensive feedback

Summary

This is a well-executed PR that improves the Input component's sizing, alignment, and token usage. The code quality is high and follows EDS conventions. I found a few minor issues and suggestions for improvement.

Issues Found

1. Potential CSS Variable Override Risk (input.css:103)

The --_placeholder-text-color alias is defined on line 103 in the readonly state block, but it's referenced on line 139 in the main .eds-input block. This creates a scoping issue where the placeholder color won't work correctly in readonly state.

Problem: The variable is defined too late in the cascade and within a parent selector scope.

Fix: Move the variable definition to line 124 (inside .eds-input block) where it's actually used, or keep it in the container block with the other aliases.

Fix this →

2. Missing Test for readOnly + invalid State (Input.test.tsx)

The tone logic now includes readOnly (line 26 in Input.tsx), but there's no test verifying that readOnly + invalid state shows neutral appearance instead of danger.

Suggestion: Add a test case:

it('Sets neutral appearance when readOnly and invalid', () => {
  render(<Input invalid readOnly />)
  const wrapper = getInputWrapper()
  expect(wrapper).toHaveAttribute('data-color-appearance', 'neutral')
})

Fix this →

Strengths

  1. Excellent Breaking Change Handling: The API rename from showErrorIcon to hideErrorIcon is more intuitive and properly updated across all files
  2. Comprehensive Testing: All 42 tests passing, with good coverage of the new hideErrorIcon behavior
  3. Token Usage: Good use of semantic token aliases (--_bg-input, --_readonly-text-color) with clear comments about temporary nature
  4. Accessibility: Maintains WCAG compliance, error icon properly hidden in disabled/readonly states
  5. CSS Quality: Modern logical properties (margin-inline-start/end), clean BEM naming, good use of :has() selector
  6. Documentation: Stories well-organized with clear categories, good examples including search patterns

Code Quality Observations

Positive

  • Clean separation of concerns in component logic
  • Proper TypeScript typing with JSDoc
  • Safari autofill overrides properly handled
  • Button sizing constraints well-documented and implemented
  • Density support correctly implemented using CSS selectors

Minor Suggestions

  1. CSS Comments (input.css:178-179): The comment "Negative margin aligns buttons with icon adornments" could be more specific about why this is needed (button size vs icon size mismatch).

  2. Magic Number (input.css:168): The 20px constraint for buttons could use a comment explaining it matches --eds-typography-ui-body-md-line-height-default token value.

  3. Test Organization: Consider adding a dedicated describe('hideErrorIcon') block since it's a new API feature, rather than mixing it into "Error icon" tests.

Accessibility Review

  • ✅ Error icon properly uses aria-invalid attribute
  • ✅ Error icon correctly hidden in disabled/readonly states
  • ✅ No accessibility violations expected (axe tests should pass)
  • ✅ Semantic HTML maintained
  • ✅ Focus states properly styled

Breaking Changes

API Change: showErrorIconhideErrorIcon

  • Impact: Low - this is a beta component (@equinor/eds-core-react/next)
  • Migration: Users need to flip boolean logic when upgrading
  • Recommendation: Document this in release notes/changelog

Performance

No performance concerns. The CSS changes use efficient selectors and the component logic is straightforward.

Recommendations

  1. Fix the CSS variable scoping issue (priority: medium)
  2. Add test for readOnly + invalid state (priority: low)
  3. Consider adding a comment explaining the 20px button constraint (priority: low)

Verdict

Approved with minor suggestions. The PR is ready to merge after addressing the CSS variable scoping issue. The other suggestions are optional improvements that can be addressed in follow-up work if desired.

Great work on improving the Input component! The sizing fixes and token usage improvements are solid contributions to EDS 2.0.


- Add showErrorIcon prop to optionally hide error icon (for Search pattern)
- Fix button alignment in adornments (constrain to 20x20px)
- Fix adornment colors to stay subtle in all states
- Update comfortable density to 24px height (from 28px)
- Add negative margins to align buttons with icon adornments
- Remove data-font-family from adornment wrappers
- Use tokens instead of hardcoded values where possible
- Update stories with Search patterns and European currency examples
- Add comprehensive tests for showErrorIcon behavior
- Update documentation to remove incorrect color inheritance claims

Fixes #4522

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@millus
millus force-pushed the fix/input-adjustments-4522 branch from 47b80a7 to 737c27b Compare February 18, 2026 11:01
- Remove incorrect placeholder color override in readonly state (lines 102-106)
  This was creating a CSS variable scoping issue where placeholder used
  the default color instead of readonly color
- Add test for readonly + invalid showing neutral appearance
- Add test for error icon hidden in readonly + invalid state

Addresses review feedback on PR #4531

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

@pomfrida pomfrida left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice job improving, looking good ✨

A few observations for future iterations (not blocking):

  • Overriding Button size/padding from Input's CSS is a reasonable approach for the current use case, though a dedicated small button variant could be cleaner long-term (already discussed but will need a more thorough plan how to do this)
  • data-invalid still being set on the container when readOnly is harmless since the readonly CSS rules come after and override the invalid styles — just worth being aware of if the CSS is eve reordered
  • Tests using container.querySelector('.eds-error-icon') work fine; adding a data-testid would be a nice-to-have to avoid the eslint-disable comments

@millus
millus merged commit 7564780 into main Feb 20, 2026
9 checks passed
@millus
millus deleted the fix/input-adjustments-4522 branch February 20, 2026 08:16
@github-actions github-actions Bot mentioned this pull request Feb 20, 2026
@github-actions github-actions Bot mentioned this pull request Apr 8, 2026
@github-actions github-actions Bot mentioned this pull request May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix - Input - some changes in design that needs to be reflected in code

2 participants