fix(ui): fix and improve Checkbox and Radio layout, markup, and props routing#1772
Open
franzheidl wants to merge 5 commits into
Hidden character warning
The head ref may contain hidden characters: "franz-fix-checkbox\u2014radio-1753-1754"
Open
fix(ui): fix and improve Checkbox and Radio layout, markup, and props routing#1772franzheidl wants to merge 5 commits into
franzheidl wants to merge 5 commits into
Conversation
* Position native input absolutely within the mock wrapper to eliminate implied height inflation * Make sure input has the exact same dimensions as the mock wrapper * Route ...props and className to the native input * Update HTML attributes type accordingly * Introduce wrapperClassName for outer container styling, consistent with other components * Update and add tests accordingly Signed-off-by: Franz Heidl <f.heidl@sap.com>
* Position native input absolutely within the mock wrapper to eliminate implied height inflation * Make sure input has the exact same dimensions as the mock wrapper * Route ...props and className to the native input * Update HTML attributes type accordingly * Introduce wrapperClassName for outer container styling, consistent with other components * Fix copy-paste bug: correct juno-checkbox-group-options to juno-radio-group-options in RadioGroup * Update and add tests accordingly Signed-off-by: Franz Heidl <f.heidl@sap.com>
* Remove jn:leading-0 from labelStyles in Checkbox and Radio as it was breaking the required marker position and label alignment * Add jn:leading-[0] to the outer container div instead, which is the correct place to suppress inherited line-height from parent contexts * Add explanatory comments to style constants in both components to document the non-obvious CSS layout decisions Signed-off-by: Franz Heidl <f.heidl@sap.com>
🦋 Changeset detectedLatest commit: 30f132a The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
- use plain strings for classNames where possible Signed-off-by: Franz Heidl <f.heidl@sap.com>
franzheidl
added
the
ui-components
Signed-off-by: Franz Heidl <f.heidl@sap.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the Checkbox and Radio components in @cloudoperators/juno-ui-components to correct vertical alignment issues, improve semantic markup by routing props to the native <input>, and align the public API with a new wrapperClassName prop. It also fixes a RadioGroup class name typo and updates/adds tests to reflect the new props routing behavior.
Changes:
- Fix layout/alignment by overlaying the native
<input>(absolute inset-0) and applyingleading-0to the outer container to avoid inherited line-height inflating height. - Route
...propsandclassNameto the native<input>; introducewrapperClassNamefor styling the mock wrapper; update prop types accordingly. - Fix
RadioGroupoption container class name and update/add tests + add a changeset.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ui-components/src/components/Checkbox/Checkbox.component.tsx | Updates markup/layout and routes props to the native checkbox input; adds wrapperClassName. |
| packages/ui-components/src/components/Checkbox/Checkbox.test.tsx | Updates tests for className/prop forwarding to the input and adds wrapperClassName coverage. |
| packages/ui-components/src/components/Radio/Radio.component.tsx | Updates markup/layout and routes props to the native radio input; adds wrapperClassName. |
| packages/ui-components/src/components/Radio/Radio.test.tsx | Updates tests for className/prop forwarding to the input and adds wrapperClassName/ARIA coverage. |
| packages/ui-components/src/components/RadioGroup/RadioGroup.component.tsx | Fixes a copy/paste class name (juno-checkbox-group-options → juno-radio-group-options). |
| .changeset/little-shoes-tan.md | Adds a patch changeset for the UI-components package. |
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.
Summary
Addresses layout, markup, and API issues in the Checkbox and Radio components.
Changes Made
Layout & alignment
The native
<input>element in bothCheckboxandRadiocomponents was in normal flow inside the mock wrapper<div>, causing the wrapper to exceed the intended 16x16 size due to browser-implied inline-block rendering and line-height. This resulted in the components appearing visually shifted upward in items-center flex contexts (e.g. DataGrid rows).<input>is now positionedabsolute inset-0, making it an overlay on the mock with the same dimensions (even if we change them at some point in the future), but not influencing/disturbing element flowleading-0added to the outer container<div>to eliminate inherited line-height from parent contexts, which was the remaining source of implicit heightProps routing
Arbitrary
...propswere spread onto the decorative mock div (div.juno-checkbox/div.juno-radio) rather than the native<input>element before. Attributes likearia-label,aria-describedby, anddata-*therefore landed on the wrong element – semantically incorrect and broken for accessibility tooling. Arbitrary props are now re-routed to the actual<input>element:...propsandclassNameare now forwarded to the native<input>elementCheckboxPropsupdated fromHTMLAttributes<HTMLDivElement>toInputHTMLAttributes<HTMLInputElement>RadioPropsupdated fromOmit<HTMLAttributes<HTMLDivElement>, "onChange"> toOmit<InputHTMLAttributes<HTMLInputElement>, "onChange">, preserving the customonChangesignaturewrapperClassNameprop introduced on both components for styling the outer container, consistent with the pattern used across the library (TextInput,Switch,Select,Textarea,ComboBox, etc.)Additional fixes
useIdwas called inside a nested helper function before, in effect violating Rules of Hooks , theuseIdcall now lives in an unconditional top-level call in bothCheckboxandRadio(this is a problem spread across our components)RadioGroupcorrected: inner options container was incorrectly classedjuno-checkbox-group-optionsinstead ofjuno-radio-group-options, this is fixed nowTests
Existing tests updated to reflect the new
classNameforwarding target; new tests added forwrapperClassName,aria-labelforwarding, and arbitrary prop forwarding.Related Issues
Fixes #1714, closes #1753, closes #1754.
Screenshots
Checkbox Alignment Before:
Checkbox Alignment After:
Testing Instructions
pnpm ipnpm run test Checkbox CheckboxGroup Radio RadioGroupChecklist
PR Manifesto
Review the PR Manifesto for best practises.